From dcc9cc13ed3d8624b15f5c83c5e40f9795566717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 25 Sep 2023 10:07:06 +0200 Subject: [PATCH] feat(core): Remove `storeMetadata` and `getSize` from binary data manager interface (no-changelog) (#7195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Depends on: #7164 | Story: [PAY-838](https://linear.app/n8n/issue/PAY-838/introduce-object-store-service-for-binary-data) This PR removes `storeMetadata` and `getSize` from the binary data manager interface, as these are specific to filesystem mode. Also this disambiguates identifiers: ``` binaryDataId filesystem:289b4aac51e-dac6-4167-b793-6d5c415e2b47 {mode}:{fileId} fileId - FS 289b4aac51e-dac6-4167-b793-6d5c415e2b47 {executionId}{uuid} fileId - S3 /workflows/{workflowId}/executions/{executionId}/binary_data/b4aac51e-dac6-4167-b793-6d5c415e2b47 ``` Note: The object store changes originally in this PR were extracted out into the final PR. --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- .../core/src/BinaryData/BinaryData.service.ts | 98 ++++++++----------- .../core/src/BinaryData/FileSystem.manager.ts | 90 +++++++++-------- packages/core/src/BinaryData/errors.ts | 13 +++ packages/core/src/BinaryData/types.ts | 41 ++++---- packages/core/src/BinaryData/utils.ts | 15 ++- packages/core/src/NodeExecuteFunctions.ts | 6 +- packages/workflow/src/Interfaces.ts | 12 +-- 7 files changed, 143 insertions(+), 132 deletions(-) create mode 100644 packages/core/src/BinaryData/errors.ts diff --git a/packages/core/src/BinaryData/BinaryData.service.ts b/packages/core/src/BinaryData/BinaryData.service.ts index cd5b65816c..8b7d77b68b 100644 --- a/packages/core/src/BinaryData/BinaryData.service.ts +++ b/packages/core/src/BinaryData/BinaryData.service.ts @@ -5,12 +5,13 @@ import { Service } from 'typedi'; import { BINARY_ENCODING, LoggerProxy as Logger, IBinaryData } from 'n8n-workflow'; import { FileSystemManager } from './FileSystem.manager'; -import { InvalidBinaryDataManagerError, InvalidBinaryDataModeError, areValidModes } from './utils'; +import { UnknownBinaryDataManager, InvalidBinaryDataMode } from './errors'; +import { LogCatch } from '../decorators/LogCatch.decorator'; +import { areValidModes } from './utils'; import type { Readable } from 'stream'; import type { BinaryData } from './types'; import type { INodeExecutionData } from 'n8n-workflow'; -import { LogCatch } from '../decorators/LogCatch.decorator'; @Service() export class BinaryDataService { @@ -21,7 +22,7 @@ export class BinaryDataService { private managers: Record = {}; async init(config: BinaryData.Config) { - if (!areValidModes(config.availableModes)) throw new InvalidBinaryDataModeError(); + if (!areValidModes(config.availableModes)) throw new InvalidBinaryDataMode(); this.availableModes = config.availableModes; this.mode = config.mode; @@ -45,47 +46,39 @@ export class BinaryDataService { return binaryData; } - const identifier = await manager.copyByPath(path, executionId); - binaryData.id = this.createIdentifier(identifier); - binaryData.data = this.mode; // clear binary data from memory - - const fileSize = await manager.getSize(identifier); - binaryData.fileSize = prettyBytes(fileSize); - - await manager.storeMetadata(identifier, { + const { fileId, fileSize } = await manager.copyByFilePath(path, executionId, { fileName: binaryData.fileName, mimeType: binaryData.mimeType, - fileSize, }); + binaryData.id = this.createBinaryDataId(fileId); + binaryData.fileSize = prettyBytes(fileSize); + binaryData.data = this.mode; // clear binary data from memory + return binaryData; } @LogCatch((error) => Logger.error('Failed to write binary data file', { error })) - async store(binaryData: IBinaryData, input: Buffer | Readable, executionId: string) { + async store(binaryData: IBinaryData, bufferOrStream: Buffer | Readable, executionId: string) { const manager = this.managers[this.mode]; if (!manager) { - const buffer = await this.binaryToBuffer(input); + const buffer = await this.binaryToBuffer(bufferOrStream); binaryData.data = buffer.toString(BINARY_ENCODING); binaryData.fileSize = prettyBytes(buffer.length); return binaryData; } - const identifier = await manager.store(input, executionId); - binaryData.id = this.createIdentifier(identifier); - binaryData.data = this.mode; // clear binary data from memory - - const fileSize = await manager.getSize(identifier); - binaryData.fileSize = prettyBytes(fileSize); - - await manager.storeMetadata(identifier, { + const { fileId, fileSize } = await manager.store(bufferOrStream, executionId, { fileName: binaryData.fileName, mimeType: binaryData.mimeType, - fileSize, }); + binaryData.id = this.createBinaryDataId(fileId); + binaryData.fileSize = prettyBytes(fileSize); + binaryData.data = this.mode; // clear binary data from memory + return binaryData; } @@ -96,34 +89,32 @@ export class BinaryDataService { }); } - getAsStream(identifier: string, chunkSize?: number) { - const { mode, id } = this.splitBinaryModeFileId(identifier); + getAsStream(binaryDataId: string, chunkSize?: number) { + const [mode, fileId] = binaryDataId.split(':'); - return this.getManager(mode).getStream(id, chunkSize); + return this.getManager(mode).getAsStream(fileId, chunkSize); } - async getBinaryDataBuffer(binaryData: IBinaryData) { - if (binaryData.id) return this.retrieveBinaryDataByIdentifier(binaryData.id); + async getAsBuffer(binaryData: IBinaryData) { + if (binaryData.id) { + const [mode, fileId] = binaryData.id.split(':'); + + return this.getManager(mode).getAsBuffer(fileId); + } return Buffer.from(binaryData.data, BINARY_ENCODING); } - async retrieveBinaryDataByIdentifier(identifier: string) { - const { mode, id } = this.splitBinaryModeFileId(identifier); + getPath(binaryDataId: string) { + const [mode, fileId] = binaryDataId.split(':'); - return this.getManager(mode).getBuffer(id); + return this.getManager(mode).getPath(fileId); } - getPath(identifier: string) { - const { mode, id } = this.splitBinaryModeFileId(identifier); + async getMetadata(binaryDataId: string) { + const [mode, fileId] = binaryDataId.split(':'); - return this.getManager(mode).getPath(id); - } - - async getMetadata(identifier: string) { - const { mode, id } = this.splitBinaryModeFileId(identifier); - - return this.getManager(mode).getMetadata(id); + return this.getManager(mode).getMetadata(fileId); } async deleteManyByExecutionIds(executionIds: string[]) { @@ -167,14 +158,11 @@ export class BinaryDataService { // private methods // ---------------------------------- - private createIdentifier(filename: string) { - return `${this.mode}:${filename}`; - } - - private splitBinaryModeFileId(fileId: string) { - const [mode, id] = fileId.split(':'); - - return { mode, id }; + /** + * Create an identifier `${mode}:{fileId}` for `IBinaryData['id']`. + */ + private createBinaryDataId(fileId: string) { + return `${this.mode}:${fileId}`; } private async duplicateBinaryDataInExecData( @@ -195,12 +183,12 @@ export class BinaryDataService { return { key, newId: undefined }; } - return manager - ?.copyByIdentifier(this.splitBinaryModeFileId(binaryDataId).id, executionId) - .then((filename) => ({ - newId: this.createIdentifier(filename), - key, - })); + const [_mode, fileId] = binaryDataId.split(':'); + + return manager?.copyByFileId(fileId, executionId).then((newFileId) => ({ + newId: this.createBinaryDataId(newFileId), + key, + })); }); return Promise.all(bdPromises).then((b) => { @@ -222,6 +210,6 @@ export class BinaryDataService { if (manager) return manager; - throw new InvalidBinaryDataManagerError(mode); + throw new UnknownBinaryDataManager(mode); } } diff --git a/packages/core/src/BinaryData/FileSystem.manager.ts b/packages/core/src/BinaryData/FileSystem.manager.ts index 84a86716dd..7667fd9f80 100644 --- a/packages/core/src/BinaryData/FileSystem.manager.ts +++ b/packages/core/src/BinaryData/FileSystem.manager.ts @@ -5,9 +5,9 @@ import { v4 as uuid } from 'uuid'; import { jsonParse } from 'n8n-workflow'; import { FileNotFoundError } from '../errors'; +import { ensureDirExists } from './utils'; import type { Readable } from 'stream'; -import type { BinaryMetadata } from 'n8n-workflow'; import type { BinaryData } from './types'; const EXECUTION_ID_EXTRACTOR = @@ -17,15 +17,15 @@ export class FileSystemManager implements BinaryData.Manager { constructor(private storagePath: string) {} async init() { - await this.ensureDirExists(this.storagePath); + await ensureDirExists(this.storagePath); } - getPath(identifier: string) { - return this.resolvePath(identifier); + getPath(fileId: string) { + return this.resolvePath(fileId); } - async getSize(identifier: string) { - const filePath = this.getPath(identifier); + async getSize(fileId: string) { + const filePath = this.getPath(fileId); try { const stats = await fs.stat(filePath); @@ -35,14 +35,14 @@ export class FileSystemManager implements BinaryData.Manager { } } - getStream(identifier: string, chunkSize?: number) { - const filePath = this.getPath(identifier); + getAsStream(fileId: string, chunkSize?: number) { + const filePath = this.getPath(fileId); return createReadStream(filePath, { highWaterMark: chunkSize }); } - async getBuffer(identifier: string) { - const filePath = this.getPath(identifier); + async getAsBuffer(fileId: string) { + const filePath = this.getPath(fileId); try { return await fs.readFile(filePath); @@ -51,29 +51,31 @@ export class FileSystemManager implements BinaryData.Manager { } } - async storeMetadata(identifier: string, metadata: BinaryMetadata) { - const filePath = this.resolvePath(`${identifier}.metadata`); - - await fs.writeFile(filePath, JSON.stringify(metadata), { encoding: 'utf-8' }); - } - - async getMetadata(identifier: string): Promise { - const filePath = this.resolvePath(`${identifier}.metadata`); + async getMetadata(fileId: string): Promise { + const filePath = this.resolvePath(`${fileId}.metadata`); return jsonParse(await fs.readFile(filePath, { encoding: 'utf-8' })); } - async store(binaryData: Buffer | Readable, executionId: string) { - const identifier = this.createIdentifier(executionId); - const filePath = this.getPath(identifier); + async store( + binaryData: Buffer | Readable, + executionId: string, + { mimeType, fileName }: BinaryData.PreWriteMetadata, + ) { + const fileId = this.createFileId(executionId); + const filePath = this.getPath(fileId); await fs.writeFile(filePath, binaryData); - return identifier; + const fileSize = await this.getSize(fileId); + + await this.storeMetadata(fileId, { mimeType, fileName, fileSize }); + + return { fileId, fileSize }; } - async deleteOne(identifier: string) { - const filePath = this.getPath(identifier); + async deleteOne(fileId: string) { + const filePath = this.getPath(fileId); return fs.rm(filePath); } @@ -98,35 +100,35 @@ export class FileSystemManager implements BinaryData.Manager { return deletedIds; } - async copyByPath(filePath: string, executionId: string) { - const identifier = this.createIdentifier(executionId); + async copyByFilePath( + filePath: string, + executionId: string, + { mimeType, fileName }: BinaryData.PreWriteMetadata, + ) { + const newFileId = this.createFileId(executionId); - await fs.cp(filePath, this.getPath(identifier)); + await fs.cp(filePath, this.getPath(newFileId)); - return identifier; + const fileSize = await this.getSize(newFileId); + + await this.storeMetadata(newFileId, { mimeType, fileName, fileSize }); + + return { fileId: newFileId, fileSize }; } - async copyByIdentifier(identifier: string, executionId: string) { - const newIdentifier = this.createIdentifier(executionId); + async copyByFileId(fileId: string, executionId: string) { + const newFileId = this.createFileId(executionId); - await fs.copyFile(this.resolvePath(identifier), this.resolvePath(newIdentifier)); + await fs.copyFile(this.resolvePath(fileId), this.resolvePath(newFileId)); - return newIdentifier; + return newFileId; } // ---------------------------------- // private methods // ---------------------------------- - private async ensureDirExists(dir: string) { - try { - await fs.access(dir); - } catch { - await fs.mkdir(dir, { recursive: true }); - } - } - - private createIdentifier(executionId: string) { + private createFileId(executionId: string) { return [executionId, uuid()].join(''); } @@ -139,4 +141,10 @@ export class FileSystemManager implements BinaryData.Manager { return returnPath; } + + private async storeMetadata(fileId: string, metadata: BinaryData.Metadata) { + const filePath = this.resolvePath(`${fileId}.metadata`); + + await fs.writeFile(filePath, JSON.stringify(metadata), { encoding: 'utf-8' }); + } } diff --git a/packages/core/src/BinaryData/errors.ts b/packages/core/src/BinaryData/errors.ts new file mode 100644 index 0000000000..dc52875b2a --- /dev/null +++ b/packages/core/src/BinaryData/errors.ts @@ -0,0 +1,13 @@ +import { BINARY_DATA_MODES } from './utils'; + +export class InvalidBinaryDataMode extends Error { + constructor() { + super(`Invalid binary data mode. Valid modes: ${BINARY_DATA_MODES.join(', ')}`); + } +} + +export class UnknownBinaryDataManager extends Error { + constructor(mode: string) { + super(`No binary data manager found for: ${mode}`); + } +} diff --git a/packages/core/src/BinaryData/types.ts b/packages/core/src/BinaryData/types.ts index e6bc3f6ced..368e3717d4 100644 --- a/packages/core/src/BinaryData/types.ts +++ b/packages/core/src/BinaryData/types.ts @@ -1,5 +1,4 @@ import type { Readable } from 'stream'; -import type { BinaryMetadata } from 'n8n-workflow'; import type { BINARY_DATA_MODES } from './utils'; export namespace BinaryData { @@ -11,31 +10,39 @@ export namespace BinaryData { localStoragePath: string; }; + export type Metadata = { + fileName?: string; + mimeType?: string; + fileSize: number; + }; + + export type PreWriteMetadata = Omit; + export interface Manager { init(): Promise; - store(binaryData: Buffer | Readable, executionId: string): Promise; - getPath(identifier: string): string; + store( + binaryData: Buffer | Readable, + executionId: string, + preStoreMetadata: PreWriteMetadata, + ): Promise<{ fileId: string; fileSize: number }>; - // @TODO: Refactor to use identifier - getSize(path: string): Promise; - - getBuffer(identifier: string): Promise; - getStream(identifier: string, chunkSize?: number): Readable; - - // @TODO: Refactor out - not needed for object storage - storeMetadata(identifier: string, metadata: BinaryMetadata): Promise; - - // @TODO: Refactor out - not needed for object storage - getMetadata(identifier: string): Promise; + getPath(fileId: string): string; + getAsBuffer(fileId: string): Promise; + getAsStream(fileId: string, chunkSize?: number): Readable; + getMetadata(fileId: string): Promise; // @TODO: Refactor to also use `workflowId` to support full path-like identifier: // `workflows/{workflowId}/executions/{executionId}/binary_data/{fileId}` - copyByPath(path: string, executionId: string): Promise; + copyByFilePath( + path: string, + executionId: string, + metadata: PreWriteMetadata, + ): Promise<{ fileId: string; fileSize: number }>; - copyByIdentifier(identifier: string, prefix: string): Promise; + copyByFileId(fileId: string, prefix: string): Promise; - deleteOne(identifier: string): Promise; + deleteOne(fileId: string): Promise; // @TODO: Refactor to also receive `workflowId` to support full path-like identifier: // `workflows/{workflowId}/executions/{executionId}/binary_data/{fileId}` diff --git a/packages/core/src/BinaryData/utils.ts b/packages/core/src/BinaryData/utils.ts index c2bea73850..60c7668a1f 100644 --- a/packages/core/src/BinaryData/utils.ts +++ b/packages/core/src/BinaryData/utils.ts @@ -1,3 +1,4 @@ +import fs from 'fs/promises'; import type { BinaryData } from './types'; /** @@ -12,14 +13,10 @@ export function areValidModes(modes: string[]): modes is BinaryData.Mode[] { return modes.every((m) => BINARY_DATA_MODES.includes(m as BinaryData.Mode)); } -export class InvalidBinaryDataModeError extends Error { - constructor() { - super(`Invalid binary data mode. Valid modes: ${BINARY_DATA_MODES.join(', ')}`); - } -} - -export class InvalidBinaryDataManagerError extends Error { - constructor(mode: string) { - super('No binary data manager found for mode: ' + mode); +export async function ensureDirExists(dir: string) { + try { + await fs.access(dir); + } catch { + await fs.mkdir(dir, { recursive: true }); } } diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 388ae31493..36bd2660b3 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -39,7 +39,6 @@ import pick from 'lodash/pick'; import { extension, lookup } from 'mime-types'; import type { BinaryHelperFunctions, - BinaryMetadata, FieldType, FileSystemHelperFunctions, FunctionsBase, @@ -140,6 +139,7 @@ import { import { getSecretsProxy } from './Secrets'; import { getUserN8nFolderPath } from './UserSettings'; import Container from 'typedi'; +import type { BinaryData } from './BinaryData/types'; axios.defaults.timeout = 300000; // Prevent axios from adding x-form-www-urlencoded headers by default @@ -947,7 +947,7 @@ export function getBinaryPath(binaryDataId: string): string { /** * Returns binary file metadata */ -export async function getBinaryMetadata(binaryDataId: string): Promise { +export async function getBinaryMetadata(binaryDataId: string): Promise { return Container.get(BinaryDataService).getMetadata(binaryDataId); } @@ -992,7 +992,7 @@ export async function getBinaryDataBuffer( inputIndex: number, ): Promise { const binaryData = inputData.main[inputIndex]![itemIndex]!.binary![propertyName]!; - return Container.get(BinaryDataService).getBinaryDataBuffer(binaryData); + return Container.get(BinaryDataService).getAsBuffer(binaryData); } /** diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index d29c51054d..0505be1105 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -48,12 +48,6 @@ export interface IBinaryData { id?: string; } -export interface BinaryMetadata { - fileName?: string; - mimeType?: string; - fileSize: number; -} - // All properties in this interface except for // "includeCredentialsOnRefreshOnBody" will get // removed once we add the OAuth2 hooks to the @@ -694,7 +688,11 @@ export interface BinaryHelperFunctions { binaryToBuffer(body: Buffer | Readable): Promise; getBinaryPath(binaryDataId: string): string; getBinaryStream(binaryDataId: string, chunkSize?: number): Readable; - getBinaryMetadata(binaryDataId: string): Promise; + getBinaryMetadata(binaryDataId: string): Promise<{ + fileName?: string; + mimeType?: string; + fileSize: number; + }>; } export interface NodeHelperFunctions {