From 2ca742cb1582352459b9bdfe5ae8a98f6c668132 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 15 Apr 2025 10:32:38 +0200 Subject: [PATCH] refactor(core): Migrate binary-data config to a decorated config class (#14616) --- packages/@n8n/config/src/decorators.ts | 10 +-- packages/@n8n/di/src/__tests__/di.test.ts | 13 ++++ packages/@n8n/di/src/di.ts | 32 +++++---- packages/cli/src/commands/base-command.ts | 9 +-- packages/cli/src/config/schema.ts | 32 --------- packages/cli/src/config/types.ts | 2 - packages/cli/src/config/utils.ts | 14 ---- .../cli/src/errors/not-string-array.error.ts | 7 -- .../__tests__/telemetry-event-relay.test.ts | 9 ++- .../events/relays/telemetry.event-relay.ts | 10 +-- .../execution-lifecycle-hooks.test.ts | 4 +- .../__tests__/restore-binary-data-id.test.ts | 14 ++-- .../restore-binary-data-id.ts | 9 +-- packages/cli/src/services/frontend.service.ts | 11 +-- .../test/integration/shared/utils/index.ts | 7 +- .../__tests__/binary-data-service.test.ts | 21 +----- .../__tests__/binary-data.config.test.ts | 72 +++++++++++++++++++ .../src/binary-data/binary-data.config.ts | 38 +++++++++- .../src/binary-data/binary-data.service.ts | 20 ++---- packages/core/src/binary-data/index.ts | 1 + packages/core/src/binary-data/types.ts | 6 -- .../__tests__/binary-helper-functions.test.ts | 26 +++---- packages/nodes-base/test/nodes/Helpers.ts | 7 +- 23 files changed, 208 insertions(+), 166 deletions(-) delete mode 100644 packages/cli/src/config/utils.ts delete mode 100644 packages/cli/src/errors/not-string-array.error.ts create mode 100644 packages/core/src/binary-data/__tests__/binary-data.config.test.ts diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index 1e0ee90fe0..4ee9b7e665 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -27,8 +27,10 @@ const readEnv = (envName: string) => { }; export const Config: ClassDecorator = (ConfigClass: Class) => { - const factory = function () { - const config = new (ConfigClass as new () => Record)(); + const factory = function (...args: unknown[]) { + const config = new (ConfigClass as new (...a: unknown[]) => Record)( + ...args, + ); const classMetadata = globalMetadata.get(ConfigClass); if (!classMetadata) { // eslint-disable-next-line n8n-local-rules/no-plain-errors @@ -102,8 +104,8 @@ export const Env = globalMetadata.get(ConfigClass) ?? new Map(); const type = Reflect.getMetadata('design:type', target, key) as PropertyType; - const isEnum = schema instanceof z.ZodEnum; - if (type === Object && !isEnum) { + const isZodSchema = schema instanceof z.ZodType; + if (type === Object && !isZodSchema) { // eslint-disable-next-line n8n-local-rules/no-plain-errors throw new Error( `Invalid decorator metadata on key "${key as string}" on ${ConfigClass.name}\n Please use explicit typing on all config fields`, diff --git a/packages/@n8n/di/src/__tests__/di.test.ts b/packages/@n8n/di/src/__tests__/di.test.ts index 736ff06152..ccf5476c9b 100644 --- a/packages/@n8n/di/src/__tests__/di.test.ts +++ b/packages/@n8n/di/src/__tests__/di.test.ts @@ -159,6 +159,19 @@ describe('DI Container', () => { expect(() => Container.get(ErrorFactoryService)).toThrow('Factory error'); }); + + it('should handle factory with dependencies', () => { + const factory = jest.fn().mockReturnValue({}); + + @Service({ factory }) + class FactoryWithDependencies { + constructor(readonly simpleService: SimpleService) {} + } + + const instance = Container.get(FactoryWithDependencies); + expect(instance.simpleService).toBeUndefined(); + expect(factory).toHaveBeenCalledWith(Container.get(SimpleService)); + }); }); describe('instance management', () => { diff --git a/packages/@n8n/di/src/di.ts b/packages/@n8n/di/src/di.ts index 08d86eae07..61e940db5a 100644 --- a/packages/@n8n/di/src/di.ts +++ b/packages/@n8n/di/src/di.ts @@ -11,13 +11,15 @@ type AbstractConstructable = abstract new (...args: unknown[]) => T type ServiceIdentifier = Constructable | AbstractConstructable; +type Factory = (...args: unknown[]) => T; + interface Metadata { instance?: T; - factory?: () => T; + factory?: Factory; } interface Options { - factory?: () => T; + factory?: Factory; } const instances = new Map(); @@ -84,20 +86,20 @@ class ContainerClass { try { let instance: T; - if (metadata?.factory) { - instance = metadata.factory(); - } else { - const paramTypes = (Reflect.getMetadata('design:paramtypes', type) ?? - []) as Constructable[]; + const paramTypes = (Reflect.getMetadata('design:paramtypes', type) ?? []) as Constructable[]; - const dependencies = paramTypes.map(

(paramType: Constructable

, index: number) => { - if (paramType === undefined) { - throw new DIError( - `Circular dependency detected in ${type.name} at index ${index}.\n${resolutionStack.map((t) => t.name).join(' -> ')}\n`, - ); - } - return this.get(paramType); - }); + const dependencies = paramTypes.map(

(paramType: Constructable

, index: number) => { + if (paramType === undefined) { + throw new DIError( + `Circular dependency detected in ${type.name} at index ${index}.\n${resolutionStack.map((t) => t.name).join(' -> ')}\n`, + ); + } + return this.get(paramType); + }); + + if (metadata?.factory) { + instance = metadata.factory(...dependencies); + } else { // Create new instance with resolved dependencies instance = new (type as Constructable)(...dependencies) as T; } diff --git a/packages/cli/src/commands/base-command.ts b/packages/cli/src/commands/base-command.ts index 0d745e01d3..376c78dcc1 100644 --- a/packages/cli/src/commands/base-command.ts +++ b/packages/cli/src/commands/base-command.ts @@ -3,6 +3,7 @@ import { GlobalConfig } from '@n8n/config'; import { Container } from '@n8n/di'; import { Command, Errors } from '@oclif/core'; import { + BinaryDataConfig, BinaryDataService, InstanceSettings, Logger, @@ -182,8 +183,9 @@ export abstract class BaseCommand extends Command { } async initObjectStoreService() { - const isSelected = config.getEnv('binaryDataManager.mode') === 's3'; - const isAvailable = config.getEnv('binaryDataManager.availableModes').includes('s3'); + const binaryDataConfig = Container.get(BinaryDataConfig); + const isSelected = binaryDataConfig.mode === 's3'; + const isAvailable = binaryDataConfig.availableModes.includes('s3'); if (!isSelected) return; @@ -220,8 +222,7 @@ export abstract class BaseCommand extends Command { process.exit(1); } - const binaryDataConfig = config.getEnv('binaryDataManager'); - await Container.get(BinaryDataService).init(binaryDataConfig); + await Container.get(BinaryDataService).init(); } protected async initDataDeduplicationService() { diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index b7a9daa23d..bd9ee9016c 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -1,16 +1,5 @@ import { GlobalConfig } from '@n8n/config'; import { Container } from '@n8n/di'; -import convict from 'convict'; -import { InstanceSettings } from 'n8n-core'; -import path from 'path'; - -import { ensureStringArray } from './utils'; - -convict.addFormat({ - name: 'comma-separated-list', - coerce: (rawStr: string) => rawStr.split(','), - validate: ensureStringArray, -}); export const schema = { executions: { @@ -183,27 +172,6 @@ export const schema = { env: 'EXTERNAL_FRONTEND_HOOKS_URLS', }, - binaryDataManager: { - availableModes: { - format: 'comma-separated-list', - default: 'filesystem', - env: 'N8N_AVAILABLE_BINARY_DATA_MODES', - doc: 'Available modes of binary data storage, as comma separated strings', - }, - mode: { - format: ['default', 'filesystem', 's3'] as const, - default: 'default', - env: 'N8N_DEFAULT_BINARY_DATA_MODE', - doc: 'Storage mode for binary data', - }, - localStoragePath: { - format: String, - default: path.join(Container.get(InstanceSettings).n8nFolder, 'binaryData'), - env: 'N8N_BINARY_DATA_STORAGE_PATH', - doc: 'Path for binary data storage in "filesystem" mode', - }, - }, - deployment: { type: { format: String, diff --git a/packages/cli/src/config/types.ts b/packages/cli/src/config/types.ts index 33fff9b946..20e620a3b3 100644 --- a/packages/cli/src/config/types.ts +++ b/packages/cli/src/config/types.ts @@ -1,5 +1,4 @@ import type { RedisOptions } from 'ioredis'; -import type { BinaryData } from 'n8n-core'; import type { IProcessedDataConfig } from 'n8n-workflow'; import type { schema } from './schema'; @@ -76,7 +75,6 @@ type ToReturnType = T extends NumericPath type ExceptionPaths = { 'queue.bull.redis': RedisOptions; - binaryDataManager: BinaryData.Config; processedDataManager: IProcessedDataConfig; 'userManagement.isInstanceOwnerSetUp': boolean; 'ui.banners.dismissed': string[] | undefined; diff --git a/packages/cli/src/config/utils.ts b/packages/cli/src/config/utils.ts deleted file mode 100644 index decc1be5e8..0000000000 --- a/packages/cli/src/config/utils.ts +++ /dev/null @@ -1,14 +0,0 @@ -import type { SchemaObj } from 'convict'; -import { UserError } from 'n8n-workflow'; - -import { NotStringArrayError } from '@/errors/not-string-array.error'; - -export const ensureStringArray = (values: string[], { env }: SchemaObj) => { - if (!env) throw new UserError('Missing env', { extra: { env } }); - - if (!Array.isArray(values)) throw new NotStringArrayError(env); - - for (const value of values) { - if (typeof value !== 'string') throw new NotStringArrayError(env); - } -}; diff --git a/packages/cli/src/errors/not-string-array.error.ts b/packages/cli/src/errors/not-string-array.error.ts deleted file mode 100644 index 7a9c2f1bc6..0000000000 --- a/packages/cli/src/errors/not-string-array.error.ts +++ /dev/null @@ -1,7 +0,0 @@ -import { UserError } from 'n8n-workflow'; - -export class NotStringArrayError extends UserError { - constructor(env: string) { - super(`${env} is not a string array.`); - } -} diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index 1b063af8bf..b99074335b 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -1,6 +1,6 @@ import type { GlobalConfig } from '@n8n/config'; import { mock } from 'jest-mock-extended'; -import { InstanceSettings } from 'n8n-core'; +import { type BinaryDataConfig, InstanceSettings } from 'n8n-core'; import type { INode, INodesGraphResult } from 'n8n-workflow'; import { NodeApiError, TelemetryHelpers, type IRun, type IWorkflowBase } from 'n8n-workflow'; @@ -49,6 +49,10 @@ describe('TelemetryEventRelay', () => { outputs: ['console'], }, }); + const binaryDataConfig = mock({ + mode: 'default', + availableModes: ['default', 'filesystem', 's3'], + }); const instanceSettings = mockInstance(InstanceSettings, { isDocker: false, n8nFolder: '/test' }); const workflowRepository = mock(); const nodeTypes = mock(); @@ -66,6 +70,7 @@ describe('TelemetryEventRelay', () => { license, globalConfig, instanceSettings, + binaryDataConfig, workflowRepository, nodeTypes, sharedWorkflowRepository, @@ -90,6 +95,7 @@ describe('TelemetryEventRelay', () => { license, globalConfig, instanceSettings, + binaryDataConfig, workflowRepository, nodeTypes, sharedWorkflowRepository, @@ -113,6 +119,7 @@ describe('TelemetryEventRelay', () => { license, globalConfig, instanceSettings, + binaryDataConfig, workflowRepository, nodeTypes, sharedWorkflowRepository, diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 1423bffbd1..c75342ac5d 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -1,7 +1,7 @@ import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import { snakeCase } from 'change-case'; -import { InstanceSettings } from 'n8n-core'; +import { BinaryDataConfig, InstanceSettings } from 'n8n-core'; import type { ExecutionStatus, INodesGraphResult, ITelemetryTrackProperties } from 'n8n-workflow'; import { TelemetryHelpers } from 'n8n-workflow'; import os from 'node:os'; @@ -31,6 +31,7 @@ export class TelemetryEventRelay extends EventRelay { private readonly license: License, private readonly globalConfig: GlobalConfig, private readonly instanceSettings: InstanceSettings, + private readonly binaryDataConfig: BinaryDataConfig, private readonly workflowRepository: WorkflowRepository, private readonly nodeTypes: NodeTypes, private readonly sharedWorkflowRepository: SharedWorkflowRepository, @@ -761,10 +762,9 @@ export class TelemetryEventRelay extends EventRelay { private async serverStarted() { const cpus = os.cpus(); - const binaryDataConfig = config.getEnv('binaryDataManager'); - const isS3Selected = config.getEnv('binaryDataManager.mode') === 's3'; - const isS3Available = config.getEnv('binaryDataManager.availableModes').includes('s3'); + const isS3Selected = this.binaryDataConfig.mode === 's3'; + const isS3Available = this.binaryDataConfig.availableModes.includes('s3'); const isS3Licensed = this.license.isBinaryDataS3Licensed(); const authenticationMethod = config.getEnv('userManagement.authenticationMethod'); @@ -801,7 +801,7 @@ export class TelemetryEventRelay extends EventRelay { executions_data_max_age: this.globalConfig.executions.pruneDataMaxAge, }, n8n_deployment_type: config.getEnv('deployment.type'), - n8n_binary_data_mode: binaryDataConfig.mode, + n8n_binary_data_mode: this.binaryDataConfig.mode, smtp_set_up: this.globalConfig.userManagement.emails.mode === 'smtp', ldap_allowed: authenticationMethod === 'ldap', saml_enabled: authenticationMethod === 'saml', diff --git a/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts b/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts index 932e972470..e00a4af035 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts @@ -6,6 +6,7 @@ import { InstanceSettings, Logger, ExecutionLifecycleHooks, + BinaryDataConfig, } from 'n8n-core'; import { ExpressionError } from 'n8n-workflow'; import type { @@ -20,7 +21,6 @@ import type { ITaskStartedData, } from 'n8n-workflow'; -import config from '@/config'; import type { Project } from '@/databases/entities/project'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; import { EventService } from '@/events/event.service'; @@ -467,7 +467,7 @@ describe('Execution Lifecycle Hooks', () => { }); it('should restore binary data IDs after workflow execution for webhooks', async () => { - config.set('binaryDataManager.mode', 'filesystem'); + mockInstance(BinaryDataConfig, { mode: 'filesystem' }); lifecycleHooks = createHooks('webhook'); (successfulRun.data.resultData.runData = { diff --git a/packages/cli/src/execution-lifecycle/__tests__/restore-binary-data-id.test.ts b/packages/cli/src/execution-lifecycle/__tests__/restore-binary-data-id.test.ts index 76ac0d4e21..4223cab817 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/restore-binary-data-id.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/restore-binary-data-id.test.ts @@ -1,7 +1,7 @@ -import { BinaryDataService } from 'n8n-core'; +import { Container } from '@n8n/di'; +import { BinaryDataConfig, BinaryDataService } from 'n8n-core'; import type { IRun } from 'n8n-workflow'; -import config from '@/config'; import { restoreBinaryDataId } from '@/execution-lifecycle/restore-binary-data-id'; import { mockInstance } from '@test/mocking'; @@ -30,10 +30,10 @@ function getDataId(run: IRun, kind: 'binary' | 'json') { const binaryDataService = mockInstance(BinaryDataService); -for (const mode of ['filesystem-v2', 's3'] as const) { +for (const mode of ['filesystem', 's3'] as const) { describe(`on ${mode} mode`, () => { beforeAll(() => { - config.set('binaryDataManager.mode', mode); + Container.get(BinaryDataConfig).mode = mode; }); afterEach(() => { @@ -168,12 +168,8 @@ for (const mode of ['filesystem-v2', 's3'] as const) { } describe('on default mode', () => { - afterEach(() => { - config.load(config.default); - }); - it('should do nothing', async () => { - config.set('binaryDataManager.mode', 'default'); + Container.get(BinaryDataConfig).mode = 'default'; const executionId = '999'; diff --git a/packages/cli/src/execution-lifecycle/restore-binary-data-id.ts b/packages/cli/src/execution-lifecycle/restore-binary-data-id.ts index 47c1031859..6cbc45d8a2 100644 --- a/packages/cli/src/execution-lifecycle/restore-binary-data-id.ts +++ b/packages/cli/src/execution-lifecycle/restore-binary-data-id.ts @@ -1,10 +1,8 @@ import { Container } from '@n8n/di'; import type { BinaryData } from 'n8n-core'; -import { BinaryDataService, Logger } from 'n8n-core'; +import { BinaryDataConfig, BinaryDataService, Logger } from 'n8n-core'; import type { IRun, WorkflowExecuteMode } from 'n8n-workflow'; -import config from '@/config'; - /** * 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. @@ -26,10 +24,7 @@ export async function restoreBinaryDataId( executionId: string, workflowExecutionMode: WorkflowExecuteMode, ) { - if ( - workflowExecutionMode !== 'webhook' || - config.getEnv('binaryDataManager.mode') === 'default' - ) { + if (workflowExecutionMode !== 'webhook' || Container.get(BinaryDataConfig).mode === 'default') { return; } diff --git a/packages/cli/src/services/frontend.service.ts b/packages/cli/src/services/frontend.service.ts index 3c0ad4f21d..1004f4496e 100644 --- a/packages/cli/src/services/frontend.service.ts +++ b/packages/cli/src/services/frontend.service.ts @@ -4,7 +4,7 @@ import { Container, Service } from '@n8n/di'; import { createWriteStream } from 'fs'; import { mkdir } from 'fs/promises'; import uniq from 'lodash/uniq'; -import { InstanceSettings, Logger } from 'n8n-core'; +import { BinaryDataConfig, InstanceSettings, Logger } from 'n8n-core'; import type { ICredentialType, INodeTypeBaseDescription } from 'n8n-workflow'; import path from 'path'; @@ -48,6 +48,7 @@ export class FrontendService { private readonly securityConfig: SecurityConfig, private readonly modulesConfig: ModulesConfig, private readonly pushConfig: PushConfig, + private readonly binaryDataConfig: BinaryDataConfig, ) { loadNodesAndCredentials.addPostProcessor(async () => await this.generateTypes()); void this.generateTypes(); @@ -104,7 +105,7 @@ export class FrontendService { timezone: this.globalConfig.generic.timezone, urlBaseWebhook: this.urlService.getWebhookBaseUrl(), urlBaseEditor: instanceBaseUrl, - binaryDataMode: config.getEnv('binaryDataManager.mode'), + binaryDataMode: this.binaryDataConfig.mode, nodeJsVersion: process.version.replace(/^v/, ''), versionCli: N8N_VERSION, concurrency: config.getEnv('executions.concurrency.productionLimit'), @@ -294,8 +295,8 @@ export class FrontendService { this.settings.easyAIWorkflowOnboarded = false; } - const isS3Selected = config.getEnv('binaryDataManager.mode') === 's3'; - const isS3Available = config.getEnv('binaryDataManager.availableModes').includes('s3'); + const isS3Selected = this.binaryDataConfig.mode === 's3'; + const isS3Available = this.binaryDataConfig.availableModes.includes('s3'); const isS3Licensed = this.license.isBinaryDataS3Licensed(); const isAiAssistantEnabled = this.license.isAiAssistantEnabled(); const isAskAiEnabled = this.license.isAskAiEnabled(); @@ -375,7 +376,7 @@ export class FrontendService { this.settings.executionMode = config.getEnv('executions.mode'); - this.settings.binaryDataMode = config.getEnv('binaryDataManager.mode'); + this.settings.binaryDataMode = this.binaryDataConfig.mode; this.settings.enterprise.projects.team.limit = this.license.getTeamProjectLimit(); diff --git a/packages/cli/test/integration/shared/utils/index.ts b/packages/cli/test/integration/shared/utils/index.ts index 54b1605f90..58be0728d9 100644 --- a/packages/cli/test/integration/shared/utils/index.ts +++ b/packages/cli/test/integration/shared/utils/index.ts @@ -1,6 +1,7 @@ import { Container } from '@n8n/di'; import { mock } from 'jest-mock-extended'; import { + BinaryDataConfig, BinaryDataService, InstanceSettings, UnrecognizedNodeTypeError, @@ -37,6 +38,7 @@ export { setupTestServer } from './test-server'; * Initialize node types. */ export async function initActiveWorkflowManager() { + mockInstance(BinaryDataConfig); mockInstance(InstanceSettings, { isMultiMain: false, }); @@ -110,12 +112,13 @@ export async function initNodeTypes(customNodes?: INodeTypeData) { * Initialize a BinaryDataService for test runs. */ export async function initBinaryDataService(mode: 'default' | 'filesystem' = 'default') { - const binaryDataService = new BinaryDataService(mock(), mock()); - await binaryDataService.init({ + const config = mock({ mode, availableModes: [mode], localStoragePath: '', }); + const binaryDataService = new BinaryDataService(config); + await binaryDataService.init(); Container.set(BinaryDataService, binaryDataService); } diff --git a/packages/core/src/binary-data/__tests__/binary-data-service.test.ts b/packages/core/src/binary-data/__tests__/binary-data-service.test.ts index d8b166c23a..f5ff6cf4fa 100644 --- a/packages/core/src/binary-data/__tests__/binary-data-service.test.ts +++ b/packages/core/src/binary-data/__tests__/binary-data-service.test.ts @@ -2,8 +2,6 @@ import { mock } from 'jest-mock-extended'; import { sign, JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken'; import type { IBinaryData } from 'n8n-workflow'; -import { type InstanceSettings } from '@/instance-settings'; - import type { BinaryDataConfig } from '../binary-data.config'; import { BinaryDataService } from '../binary-data.service'; @@ -13,7 +11,6 @@ jest.useFakeTimers({ now }); describe('BinaryDataService', () => { const signingSecret = 'test-signing-secret'; const config = mock({ signingSecret }); - const instanceSettings = mock({ encryptionKey: 'test-encryption-key' }); const binaryData = mock({ id: 'filesystem:id_123' }); const validToken = sign({ id: binaryData.id }, signingSecret, { expiresIn: '1 day' }); @@ -22,23 +19,7 @@ describe('BinaryDataService', () => { jest.resetAllMocks(); config.signingSecret = signingSecret; - service = new BinaryDataService(instanceSettings, config); - }); - - describe('constructor', () => { - it('should derive the signingSecret from the encryption-key, if not provided via BinaryDataConfig', () => { - config.signingSecret = undefined; - - const service = new BinaryDataService(instanceSettings, config); - - expect(service.signingSecret).toBe( - 'f7a78761c5cc17a2753e7e9d85d90e12de87d8131aea4479a11d1c7bb9655b20', - ); - }); - - it('should use signingSecret as provided in BinaryDataConfig', () => { - expect(service.signingSecret).toBe(signingSecret); - }); + service = new BinaryDataService(config); }); describe('createSignedToken', () => { diff --git a/packages/core/src/binary-data/__tests__/binary-data.config.test.ts b/packages/core/src/binary-data/__tests__/binary-data.config.test.ts new file mode 100644 index 0000000000..c5e6996654 --- /dev/null +++ b/packages/core/src/binary-data/__tests__/binary-data.config.test.ts @@ -0,0 +1,72 @@ +import { Container } from '@n8n/di'; + +import { InstanceSettings } from '@/instance-settings'; +import { mockInstance } from '@test/utils'; + +import { BinaryDataConfig } from '../binary-data.config'; + +describe('BinaryDataConfig', () => { + const n8nFolder = '/test/n8n'; + const encryptionKey = 'test-encryption-key'; + console.warn = jest.fn().mockImplementation(() => {}); + + const now = new Date('2025-01-01T01:23:45.678Z'); + jest.useFakeTimers({ now }); + + beforeEach(() => { + process.env = {}; + jest.resetAllMocks(); + Container.reset(); + mockInstance(InstanceSettings, { encryptionKey, n8nFolder }); + }); + + it('should use default values when no env variables are defined', () => { + const config = Container.get(BinaryDataConfig); + + expect(config.availableModes).toEqual(['filesystem']); + expect(config.mode).toBe('default'); + expect(config.localStoragePath).toBe('/test/n8n/binaryData'); + }); + + it('should use values from env variables when defined', () => { + process.env.N8N_AVAILABLE_BINARY_DATA_MODES = 'filesystem,s3'; + process.env.N8N_DEFAULT_BINARY_DATA_MODE = 's3'; + process.env.N8N_BINARY_DATA_STORAGE_PATH = '/custom/storage/path'; + process.env.N8N_BINARY_DATA_SIGNING_SECRET = 'super-secret'; + + const config = Container.get(BinaryDataConfig); + + expect(config.mode).toEqual('s3'); + expect(config.availableModes).toEqual(['filesystem', 's3']); + expect(config.localStoragePath).toEqual('/custom/storage/path'); + expect(config.signingSecret).toBe('super-secret'); + }); + + it('should derive the signing secret from the encryption-key, when none is passed in', () => { + const config = Container.get(BinaryDataConfig); + + expect(config.signingSecret).toBe('96eHYcXMF6J1Pn6dhdkOEt6H2BMa6kR5oR0ce7llWyA='); + }); + + it('should fallback to default for mode', () => { + process.env.N8N_DEFAULT_BINARY_DATA_MODE = 'invalid-mode'; + + const config = Container.get(BinaryDataConfig); + + expect(config.mode).toEqual('default'); + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining('Invalid value for N8N_DEFAULT_BINARY_DATA_MODE'), + ); + }); + + it('should fallback to default for available modes', () => { + process.env.N8N_AVAILABLE_BINARY_DATA_MODES = 'filesystem,invalid-mode,s3'; + + const config = Container.get(BinaryDataConfig); + + expect(config.availableModes).toEqual(['filesystem']); + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining('Invalid value for N8N_AVAILABLE_BINARY_DATA_MODES'), + ); + }); +}); diff --git a/packages/core/src/binary-data/binary-data.config.ts b/packages/core/src/binary-data/binary-data.config.ts index 95598cf92b..48d4391dba 100644 --- a/packages/core/src/binary-data/binary-data.config.ts +++ b/packages/core/src/binary-data/binary-data.config.ts @@ -1,8 +1,42 @@ import { Config, Env } from '@n8n/config'; +import { createHash } from 'node:crypto'; +import path from 'node:path'; +import { z } from 'zod'; + +import { InstanceSettings } from '@/instance-settings'; + +const binaryDataModesSchema = z.enum(['default', 'filesystem', 's3']); + +const availableModesSchema = z + .string() + .transform((value) => value.split(',')) + .pipe(binaryDataModesSchema.array()); @Config export class BinaryDataConfig { - /** Secret for creating publicly-accesible signed URLs for binary data */ + /** Available modes of binary data storage, as comma separated strings. */ + @Env('N8N_AVAILABLE_BINARY_DATA_MODES', availableModesSchema) + availableModes: z.infer = ['filesystem']; + + /** Storage mode for binary data. */ + @Env('N8N_DEFAULT_BINARY_DATA_MODE', binaryDataModesSchema) + mode: z.infer = 'default'; + + /** Path for binary data storage in "filesystem" mode. */ + @Env('N8N_BINARY_DATA_STORAGE_PATH') + localStoragePath: string; + + /** + * Secret for creating publicly-accesible signed URLs for binary data. + * When not passed in, this will be derived from the instances's encryption-key + **/ @Env('N8N_BINARY_DATA_SIGNING_SECRET') - signingSecret?: string = undefined; + signingSecret: string; + + constructor({ encryptionKey, n8nFolder }: InstanceSettings) { + this.localStoragePath = path.join(n8nFolder, 'binaryData'); + this.signingSecret = createHash('sha256') + .update(`url-signing:${encryptionKey}`) + .digest('base64'); + } } diff --git a/packages/core/src/binary-data/binary-data.service.ts b/packages/core/src/binary-data/binary-data.service.ts index bc28ce3d7d..b8b21d6f83 100644 --- a/packages/core/src/binary-data/binary-data.service.ts +++ b/packages/core/src/binary-data/binary-data.service.ts @@ -1,5 +1,4 @@ import { Container, Service } from '@n8n/di'; -import { createHash } from 'crypto'; import jwt from 'jsonwebtoken'; import { BINARY_ENCODING, UnexpectedError } from 'n8n-workflow'; import type { INodeExecutionData, IBinaryData } from 'n8n-workflow'; @@ -7,8 +6,6 @@ import { readFile, stat } from 'node:fs/promises'; import prettyBytes from 'pretty-bytes'; import type { Readable } from 'stream'; -import { InstanceSettings } from '@/instance-settings'; - import { BinaryDataConfig } from './binary-data.config'; import type { BinaryData } from './types'; import { areConfigModes, binaryToBuffer } from './utils'; @@ -21,15 +18,10 @@ export class BinaryDataService { private managers: Record = {}; - readonly signingSecret: string; + constructor(private readonly config: BinaryDataConfig) {} - constructor({ encryptionKey }: InstanceSettings, binaryDataConfig: BinaryDataConfig) { - this.signingSecret = - binaryDataConfig.signingSecret ?? - createHash('sha256').update(`url-signing:${encryptionKey}`).digest('hex'); - } - - async init(config: BinaryData.Config) { + async init() { + const { config } = this; if (!areConfigModes(config.availableModes)) throw new InvalidModeError(); this.mode = config.mode === 'filesystem' ? 'filesystem-v2' : config.mode; @@ -62,11 +54,13 @@ export class BinaryDataService { id: binaryData.id, }; - return jwt.sign(signingPayload, this.signingSecret, { expiresIn }); + const { signingSecret } = this.config; + return jwt.sign(signingPayload, signingSecret, { expiresIn }); } validateSignedToken(token: string) { - const signedPayload = jwt.verify(token, this.signingSecret) as BinaryData.SigningPayload; + const { signingSecret } = this.config; + const signedPayload = jwt.verify(token, signingSecret) as BinaryData.SigningPayload; return signedPayload.id; } diff --git a/packages/core/src/binary-data/index.ts b/packages/core/src/binary-data/index.ts index ef82dee30f..bdba66f7c1 100644 --- a/packages/core/src/binary-data/index.ts +++ b/packages/core/src/binary-data/index.ts @@ -1,4 +1,5 @@ export * from './binary-data.service'; +export { BinaryDataConfig } from './binary-data.config'; export * from './types'; export { ObjectStoreService } from './object-store/object-store.service.ee'; export { isStoredMode as isValidNonDefaultMode } from './utils'; diff --git a/packages/core/src/binary-data/types.ts b/packages/core/src/binary-data/types.ts index 12db5ef7ec..9ee5479277 100644 --- a/packages/core/src/binary-data/types.ts +++ b/packages/core/src/binary-data/types.ts @@ -22,12 +22,6 @@ export namespace BinaryData { */ export type StoredMode = Exclude; - export type Config = { - mode: ConfigMode; - availableModes: ConfigMode[]; - localStoragePath: string; - }; - export type Metadata = { fileName?: string; mimeType?: string; diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/binary-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/binary-helper-functions.test.ts index 5815990208..c3da999541 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/binary-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/binary-helper-functions.test.ts @@ -12,8 +12,8 @@ import { tmpdir } from 'os'; import { join } from 'path'; import { Readable } from 'stream'; +import type { BinaryDataConfig } from '@/binary-data'; import { BinaryDataService } from '@/binary-data/binary-data.service'; -import { type InstanceSettings } from '@/instance-settings'; import { assertBinaryData, @@ -38,21 +38,24 @@ const bufferToIncomingMessage = (buffer: Buffer, encoding = 'utf-8') => { }; describe('test binary data helper methods', () => { - let binaryDataService: BinaryDataService; const temporaryDir = mkdtempSync(join(tmpdir(), 'n8n')); + const binaryDataConfig = mock({ + mode: 'default', + availableModes: ['default', 'filesystem'], + localStoragePath: temporaryDir, + }); + let binaryDataService: BinaryDataService; beforeEach(() => { - binaryDataService = new BinaryDataService(mock(), mock()); + jest.resetAllMocks(); + binaryDataService = new BinaryDataService(binaryDataConfig); Container.set(BinaryDataService, binaryDataService); }); test("test getBinaryDataBuffer(...) & setBinaryDataBuffer(...) methods in 'default' mode", async () => { // Setup a 'default' binary data manager instance - await binaryDataService.init({ - mode: 'default', - availableModes: ['default'], - localStoragePath: temporaryDir, - }); + binaryDataConfig.mode = 'default'; + await binaryDataService.init(); // Set our binary data buffer const inputData: Buffer = Buffer.from('This is some binary data', 'utf8'); @@ -98,11 +101,8 @@ describe('test binary data helper methods', () => { test("test getBinaryDataBuffer(...) & setBinaryDataBuffer(...) methods in 'filesystem' mode", async () => { // Setup a 'filesystem' binary data manager instance - await binaryDataService.init({ - mode: 'filesystem', - availableModes: ['filesystem'], - localStoragePath: temporaryDir, - }); + binaryDataConfig.mode = 'filesystem'; + await binaryDataService.init(); // Set our binary data buffer const inputData: Buffer = Buffer.from('This is some binary data', 'utf8'); diff --git a/packages/nodes-base/test/nodes/Helpers.ts b/packages/nodes-base/test/nodes/Helpers.ts index 180c243f4b..77a3ae7803 100644 --- a/packages/nodes-base/test/nodes/Helpers.ts +++ b/packages/nodes-base/test/nodes/Helpers.ts @@ -3,7 +3,7 @@ import { readFileSync, readdirSync, mkdtempSync } from 'fs'; import { mock } from 'jest-mock-extended'; import { get } from 'lodash'; import { isEmpty } from 'lodash'; -import { BinaryDataService, constructExecutionMetaData } from 'n8n-core'; +import { type BinaryDataConfig, BinaryDataService, constructExecutionMetaData } from 'n8n-core'; import type { IDataObject, IExecuteFunctions, @@ -37,12 +37,13 @@ export function createTemporaryDir(prefix = 'n8n') { } export async function initBinaryDataService() { - const binaryDataService = new BinaryDataService(mock(), mock()); - await binaryDataService.init({ + const binaryDataConfig = mock({ mode: 'default', availableModes: ['default'], localStoragePath: createTemporaryDir(), }); + const binaryDataService = new BinaryDataService(binaryDataConfig); + await binaryDataService.init(); Container.set(BinaryDataService, binaryDataService); }