diff --git a/packages/twenty-server/src/engine/core-modules/core-engine.module.ts b/packages/twenty-server/src/engine/core-modules/core-engine.module.ts index 32e789491..7780ca0bd 100644 --- a/packages/twenty-server/src/engine/core-modules/core-engine.module.ts +++ b/packages/twenty-server/src/engine/core-modules/core-engine.module.ts @@ -14,12 +14,10 @@ import { TimelineCalendarEventModule } from 'src/engine/core-modules/calendar/ti import { CaptchaModule } from 'src/engine/core-modules/captcha/captcha.module'; import { captchaModuleFactory } from 'src/engine/core-modules/captcha/captcha.module-factory'; import { EmailModule } from 'src/engine/core-modules/email/email.module'; -import { emailModuleFactory } from 'src/engine/core-modules/email/email.module-factory'; import { ExceptionHandlerModule } from 'src/engine/core-modules/exception-handler/exception-handler.module'; import { exceptionHandlerModuleFactory } from 'src/engine/core-modules/exception-handler/exception-handler.module-factory'; import { FeatureFlagModule } from 'src/engine/core-modules/feature-flag/feature-flag.module'; import { FileStorageModule } from 'src/engine/core-modules/file-storage/file-storage.module'; -import { fileStorageModuleFactory } from 'src/engine/core-modules/file-storage/file-storage.module-factory'; import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; import { HealthModule } from 'src/engine/core-modules/health/health.module'; import { LabModule } from 'src/engine/core-modules/lab/lab.module'; @@ -85,10 +83,7 @@ import { FileModule } from './file/file.module'; RedisClientModule, WorkspaceQueryRunnerModule, SubscriptionsModule, - FileStorageModule.forRootAsync({ - useFactory: fileStorageModuleFactory, - inject: [TwentyConfigService], - }), + FileStorageModule.forRoot(), LoggerModule.forRootAsync({ useFactory: loggerModuleFactory, inject: [TwentyConfigService], @@ -101,10 +96,7 @@ import { FileModule } from './file/file.module'; useFactory: exceptionHandlerModuleFactory, inject: [TwentyConfigService, HttpAdapterHost], }), - EmailModule.forRoot({ - useFactory: emailModuleFactory, - inject: [TwentyConfigService], - }), + EmailModule.forRoot(), CaptchaModule.forRoot({ useFactory: captchaModuleFactory, inject: [TwentyConfigService], diff --git a/packages/twenty-server/src/engine/core-modules/email/__tests__/email-driver.factory.spec.ts b/packages/twenty-server/src/engine/core-modules/email/__tests__/email-driver.factory.spec.ts new file mode 100644 index 000000000..e94027eae --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/email/__tests__/email-driver.factory.spec.ts @@ -0,0 +1,229 @@ +import { Test, TestingModule } from '@nestjs/testing'; + +import { EmailDriverFactory } from 'src/engine/core-modules/email/email-driver.factory'; +import { EmailDriver } from 'src/engine/core-modules/email/enums/email-driver.enum'; +import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; + +describe('EmailDriverFactory', () => { + let factory: EmailDriverFactory; + let twentyConfigService: TwentyConfigService; + + const mockTwentyConfigService = { + get: jest.fn(), + }; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + EmailDriverFactory, + { + provide: TwentyConfigService, + useValue: mockTwentyConfigService, + }, + ], + }).compile(); + + factory = module.get(EmailDriverFactory); + twentyConfigService = module.get(TwentyConfigService); + + jest.clearAllMocks(); + }); + + describe('buildConfigKey', () => { + it('should return "logger" for logger driver', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(EmailDriver.Logger); + + const result = factory['buildConfigKey'](); + + expect(result).toBe('logger'); + expect(twentyConfigService.get).toHaveBeenCalledWith('EMAIL_DRIVER'); + }); + + it('should return smtp config key for smtp driver', () => { + jest.spyOn(twentyConfigService, 'get').mockReturnValue(EmailDriver.Smtp); + jest + .spyOn(factory as any, 'getConfigGroupHash') + .mockReturnValue('smtp-hash-123'); + + const result = factory['buildConfigKey'](); + + expect(result).toBe('smtp|smtp-hash-123'); + expect(twentyConfigService.get).toHaveBeenCalledWith('EMAIL_DRIVER'); + }); + + it('should throw error for unsupported driver', () => { + jest.spyOn(twentyConfigService, 'get').mockReturnValue('invalid-driver'); + + expect(() => factory['buildConfigKey']()).toThrow( + 'Unsupported email driver: invalid-driver', + ); + }); + }); + + describe('createDriver', () => { + it('should create logger driver', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(EmailDriver.Logger); + + const driver = factory['createDriver'](); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('LoggerDriver'); + }); + + it('should create smtp driver with basic configuration', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'EMAIL_DRIVER': + return EmailDriver.Smtp; + case 'EMAIL_SMTP_HOST': + return 'smtp.example.com'; + case 'EMAIL_SMTP_PORT': + return 587; + case 'EMAIL_SMTP_USER': + return undefined; + case 'EMAIL_SMTP_PASSWORD': + return undefined; + case 'EMAIL_SMTP_NO_TLS': + return false; + default: + return undefined; + } + }); + + const driver = factory['createDriver'](); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('SmtpDriver'); + }); + + it('should throw error when smtp host is missing', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'EMAIL_DRIVER': + return EmailDriver.Smtp; + case 'EMAIL_SMTP_HOST': + return undefined; + case 'EMAIL_SMTP_PORT': + return 587; + default: + return undefined; + } + }); + + expect(() => factory['createDriver']()).toThrow( + 'SMTP driver requires host and port to be defined', + ); + }); + + it('should throw error for invalid driver', () => { + jest.spyOn(twentyConfigService, 'get').mockReturnValue('invalid-driver'); + + expect(() => factory['createDriver']()).toThrow( + 'Invalid email driver: invalid-driver', + ); + }); + }); + + describe('getCurrentDriver', () => { + it('should return current driver for logger', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(EmailDriver.Logger); + + const driver = factory.getCurrentDriver(); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('LoggerDriver'); + }); + + it('should reuse driver when config key unchanged', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(EmailDriver.Logger); + + const driver1 = factory.getCurrentDriver(); + const driver2 = factory.getCurrentDriver(); + + expect(driver1).toBe(driver2); + }); + + it('should create new driver when config key changes', () => { + // First call with logger + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(EmailDriver.Logger); + + const driver1 = factory.getCurrentDriver(); + + // Second call with smtp + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'EMAIL_DRIVER': + return EmailDriver.Smtp; + case 'EMAIL_SMTP_HOST': + return 'smtp.example.com'; + case 'EMAIL_SMTP_PORT': + return 587; + default: + return undefined; + } + }); + jest + .spyOn(factory as any, 'getConfigGroupHash') + .mockReturnValue('smtp-hash-123'); + + const driver2 = factory.getCurrentDriver(); + + expect(driver1).not.toBe(driver2); + expect(driver1.constructor.name).toBe('LoggerDriver'); + expect(driver2.constructor.name).toBe('SmtpDriver'); + }); + + it('should throw error for unsupported email driver', () => { + jest.spyOn(twentyConfigService, 'get').mockReturnValue('invalid-driver'); + + expect(() => factory.getCurrentDriver()).toThrow( + 'Failed to build config key for EmailDriverFactory. Original error: Unsupported email driver: invalid-driver', + ); + }); + + it('should throw error when driver creation fails after valid config', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'EMAIL_DRIVER': + return EmailDriver.Smtp; + case 'EMAIL_SMTP_HOST': + return 'smtp.example.com'; + case 'EMAIL_SMTP_PORT': + return 587; + default: + return undefined; + } + }); + + jest + .spyOn(factory as any, 'getConfigGroupHash') + .mockReturnValue('smtp-hash-123'); + + jest.spyOn(factory as any, 'createDriver').mockImplementation(() => { + throw new Error('Driver creation failed'); + }); + + expect(() => factory.getCurrentDriver()).toThrow( + 'Failed to create driver for EmailDriverFactory with config key: smtp|smtp-hash-123. Original error: Driver creation failed', + ); + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/email/__tests__/email-sender.service.spec.ts b/packages/twenty-server/src/engine/core-modules/email/__tests__/email-sender.service.spec.ts new file mode 100644 index 000000000..39bed70ea --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/email/__tests__/email-sender.service.spec.ts @@ -0,0 +1,73 @@ +import { Test, TestingModule } from '@nestjs/testing'; + +import { SendMailOptions } from 'nodemailer'; + +import { EmailDriverFactory } from 'src/engine/core-modules/email/email-driver.factory'; +import { EmailSenderService } from 'src/engine/core-modules/email/email-sender.service'; + +describe('EmailSenderService', () => { + let service: EmailSenderService; + let emailDriverFactory: EmailDriverFactory; + + const mockEmailDriverFactory = { + getCurrentDriver: jest.fn(), + }; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + EmailSenderService, + { + provide: EmailDriverFactory, + useValue: mockEmailDriverFactory, + }, + ], + }).compile(); + + service = module.get(EmailSenderService); + emailDriverFactory = module.get(EmailDriverFactory); + + jest.clearAllMocks(); + }); + + describe('send', () => { + it('should delegate to the current driver', async () => { + const mockDriver = { + send: jest.fn().mockResolvedValue(undefined), + }; + + mockEmailDriverFactory.getCurrentDriver.mockReturnValue(mockDriver); + + const sendMailOptions: SendMailOptions = { + to: 'test@example.com', + subject: 'Test', + text: 'Test message', + }; + + await service.send(sendMailOptions); + + expect(emailDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.send).toHaveBeenCalledWith(sendMailOptions); + }); + + it('should handle driver errors', async () => { + const mockDriver = { + send: jest.fn().mockRejectedValue(new Error('Driver error')), + }; + + mockEmailDriverFactory.getCurrentDriver.mockReturnValue(mockDriver); + + const sendMailOptions: SendMailOptions = { + to: 'test@example.com', + subject: 'Test', + text: 'Test message', + }; + + await expect(service.send(sendMailOptions)).rejects.toThrow( + 'Driver error', + ); + expect(emailDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.send).toHaveBeenCalledWith(sendMailOptions); + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/email/drivers/interfaces/email-driver.interface.ts b/packages/twenty-server/src/engine/core-modules/email/drivers/interfaces/email-driver.interface.ts index a81d70071..a63874ed6 100644 --- a/packages/twenty-server/src/engine/core-modules/email/drivers/interfaces/email-driver.interface.ts +++ b/packages/twenty-server/src/engine/core-modules/email/drivers/interfaces/email-driver.interface.ts @@ -1,5 +1,5 @@ import { SendMailOptions } from 'nodemailer'; -export interface EmailDriver { +export interface EmailDriverInterface { send(sendMailOptions: SendMailOptions): Promise; } diff --git a/packages/twenty-server/src/engine/core-modules/email/drivers/logger.driver.ts b/packages/twenty-server/src/engine/core-modules/email/drivers/logger.driver.ts index c8dd1e345..a5767ed55 100644 --- a/packages/twenty-server/src/engine/core-modules/email/drivers/logger.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/email/drivers/logger.driver.ts @@ -2,9 +2,9 @@ import { Logger } from '@nestjs/common'; import { SendMailOptions } from 'nodemailer'; -import { EmailDriver } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; +import { EmailDriverInterface } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; -export class LoggerDriver implements EmailDriver { +export class LoggerDriver implements EmailDriverInterface { private readonly logger = new Logger(LoggerDriver.name); async send(sendMailOptions: SendMailOptions): Promise { diff --git a/packages/twenty-server/src/engine/core-modules/email/drivers/smtp.driver.ts b/packages/twenty-server/src/engine/core-modules/email/drivers/smtp.driver.ts index 5f325170e..6a24c6828 100644 --- a/packages/twenty-server/src/engine/core-modules/email/drivers/smtp.driver.ts +++ b/packages/twenty-server/src/engine/core-modules/email/drivers/smtp.driver.ts @@ -1,11 +1,11 @@ import { Logger } from '@nestjs/common'; -import { createTransport, Transporter, SendMailOptions } from 'nodemailer'; +import { createTransport, SendMailOptions, Transporter } from 'nodemailer'; import SMTPConnection from 'nodemailer/lib/smtp-connection'; -import { EmailDriver } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; +import { EmailDriverInterface } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; -export class SmtpDriver implements EmailDriver { +export class SmtpDriver implements EmailDriverInterface { private readonly logger = new Logger(SmtpDriver.name); private transport: Transporter; diff --git a/packages/twenty-server/src/engine/core-modules/email/email-driver.factory.ts b/packages/twenty-server/src/engine/core-modules/email/email-driver.factory.ts new file mode 100644 index 000000000..cb67404eb --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/email/email-driver.factory.ts @@ -0,0 +1,79 @@ +import { Injectable } from '@nestjs/common'; + +import { EmailDriverInterface } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; + +import { LoggerDriver } from 'src/engine/core-modules/email/drivers/logger.driver'; +import { SmtpDriver } from 'src/engine/core-modules/email/drivers/smtp.driver'; +import { EmailDriver } from 'src/engine/core-modules/email/enums/email-driver.enum'; +import { DriverFactoryBase } from 'src/engine/core-modules/twenty-config/dynamic-factory.base'; +import { ConfigVariablesGroup } from 'src/engine/core-modules/twenty-config/enums/config-variables-group.enum'; +import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; + +@Injectable() +export class EmailDriverFactory extends DriverFactoryBase { + constructor(twentyConfigService: TwentyConfigService) { + super(twentyConfigService); + } + + protected buildConfigKey(): string { + const driver = this.twentyConfigService.get('EMAIL_DRIVER'); + + if (driver === EmailDriver.Logger) { + return 'logger'; + } + + if (driver === EmailDriver.Smtp) { + const emailConfigHash = this.getConfigGroupHash( + ConfigVariablesGroup.EmailSettings, + ); + + return `smtp|${emailConfigHash}`; + } + + throw new Error(`Unsupported email driver: ${driver}`); + } + + protected createDriver(): EmailDriverInterface { + const driver = this.twentyConfigService.get('EMAIL_DRIVER'); + + switch (driver) { + case EmailDriver.Logger: + return new LoggerDriver(); + + case EmailDriver.Smtp: { + const host = this.twentyConfigService.get('EMAIL_SMTP_HOST'); + const port = this.twentyConfigService.get('EMAIL_SMTP_PORT'); + const user = this.twentyConfigService.get('EMAIL_SMTP_USER'); + const pass = this.twentyConfigService.get('EMAIL_SMTP_PASSWORD'); + const noTLS = this.twentyConfigService.get('EMAIL_SMTP_NO_TLS'); + + if (!host || !port) { + throw new Error('SMTP driver requires host and port to be defined'); + } + + const options: { + host: string; + port: number; + auth?: { user: string; pass: string }; + secure?: boolean; + ignoreTLS?: boolean; + requireTLS?: boolean; + } = { host, port }; + + if (user && pass) { + options.auth = { user, pass }; + } + + if (noTLS) { + options.secure = false; + options.ignoreTLS = true; + } + + return new SmtpDriver(options); + } + + default: + throw new Error(`Invalid email driver: ${driver}`); + } + } +} diff --git a/packages/twenty-server/src/engine/core-modules/email/email-sender.service.ts b/packages/twenty-server/src/engine/core-modules/email/email-sender.service.ts index 05306fe54..2ce92962c 100644 --- a/packages/twenty-server/src/engine/core-modules/email/email-sender.service.ts +++ b/packages/twenty-server/src/engine/core-modules/email/email-sender.service.ts @@ -1,16 +1,18 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { SendMailOptions } from 'nodemailer'; -import { EmailDriver } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; +import { EmailDriverInterface } from 'src/engine/core-modules/email/drivers/interfaces/email-driver.interface'; -import { EMAIL_DRIVER } from 'src/engine/core-modules/email/email.constants'; +import { EmailDriverFactory } from 'src/engine/core-modules/email/email-driver.factory'; @Injectable() -export class EmailSenderService implements EmailDriver { - constructor(@Inject(EMAIL_DRIVER) private driver: EmailDriver) {} +export class EmailSenderService implements EmailDriverInterface { + constructor(private readonly emailDriverFactory: EmailDriverFactory) {} async send(sendMailOptions: SendMailOptions): Promise { - await this.driver.send(sendMailOptions); + const driver = this.emailDriverFactory.getCurrentDriver(); + + await driver.send(sendMailOptions); } } diff --git a/packages/twenty-server/src/engine/core-modules/email/email.constants.ts b/packages/twenty-server/src/engine/core-modules/email/email.constants.ts deleted file mode 100644 index 45649fcb2..000000000 --- a/packages/twenty-server/src/engine/core-modules/email/email.constants.ts +++ /dev/null @@ -1 +0,0 @@ -export const EMAIL_DRIVER = Symbol('EMAIL_DRIVER'); diff --git a/packages/twenty-server/src/engine/core-modules/email/email.module-factory.ts b/packages/twenty-server/src/engine/core-modules/email/email.module-factory.ts deleted file mode 100644 index 690433bf2..000000000 --- a/packages/twenty-server/src/engine/core-modules/email/email.module-factory.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { - EmailDriver, - EmailModuleOptions, -} from 'src/engine/core-modules/email/interfaces/email.interface'; - -import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; - -export const emailModuleFactory = ( - twentyConfigService: TwentyConfigService, -): EmailModuleOptions => { - const driver = twentyConfigService.get('EMAIL_DRIVER'); - - switch (driver) { - case EmailDriver.Logger: - return { - type: EmailDriver.Logger, - }; - - case EmailDriver.Smtp: { - const options: EmailModuleOptions = { - type: EmailDriver.Smtp, - }; - - const host = twentyConfigService.get('EMAIL_SMTP_HOST'); - const port = twentyConfigService.get('EMAIL_SMTP_PORT'); - const user = twentyConfigService.get('EMAIL_SMTP_USER'); - const pass = twentyConfigService.get('EMAIL_SMTP_PASSWORD'); - const noTLS = twentyConfigService.get('EMAIL_SMTP_NO_TLS'); - - if (!host || !port) { - throw new Error( - `${driver} email driver requires host and port to be defined, check your .env file`, - ); - } - - options.host = host; - options.port = port; - - if (user && pass) options.auth = { user, pass }; - - if (noTLS) { - options.secure = false; - options.ignoreTLS = true; - } - - return options; - } - - default: - throw new Error(`Invalid email driver (${driver}), check your .env file`); - } -}; diff --git a/packages/twenty-server/src/engine/core-modules/email/email.module.ts b/packages/twenty-server/src/engine/core-modules/email/email.module.ts index 96001af7f..a5b51d93a 100644 --- a/packages/twenty-server/src/engine/core-modules/email/email.module.ts +++ b/packages/twenty-server/src/engine/core-modules/email/email.module.ts @@ -1,35 +1,17 @@ import { DynamicModule, Global } from '@nestjs/common'; -import { - EmailDriver, - EmailModuleAsyncOptions, -} from 'src/engine/core-modules/email/interfaces/email.interface'; - -import { LoggerDriver } from 'src/engine/core-modules/email/drivers/logger.driver'; -import { SmtpDriver } from 'src/engine/core-modules/email/drivers/smtp.driver'; +import { EmailDriverFactory } from 'src/engine/core-modules/email/email-driver.factory'; import { EmailSenderService } from 'src/engine/core-modules/email/email-sender.service'; -import { EMAIL_DRIVER } from 'src/engine/core-modules/email/email.constants'; import { EmailService } from 'src/engine/core-modules/email/email.service'; +import { TwentyConfigModule } from 'src/engine/core-modules/twenty-config/twenty-config.module'; @Global() export class EmailModule { - static forRoot(options: EmailModuleAsyncOptions): DynamicModule { - const provider = { - provide: EMAIL_DRIVER, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - useFactory: (...args: any[]) => { - const config = options.useFactory(...args); - - return config.type === EmailDriver.Smtp - ? new SmtpDriver(config) - : new LoggerDriver(); - }, - inject: options.inject || [], - }; - + static forRoot(): DynamicModule { return { module: EmailModule, - providers: [EmailSenderService, EmailService, provider], + imports: [TwentyConfigModule], + providers: [EmailDriverFactory, EmailSenderService, EmailService], exports: [EmailSenderService, EmailService], }; } diff --git a/packages/twenty-server/src/engine/core-modules/email/enums/email-driver.enum.ts b/packages/twenty-server/src/engine/core-modules/email/enums/email-driver.enum.ts new file mode 100644 index 000000000..bfccea435 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/email/enums/email-driver.enum.ts @@ -0,0 +1,4 @@ +export enum EmailDriver { + Logger = 'logger', + Smtp = 'smtp', +} diff --git a/packages/twenty-server/src/engine/core-modules/email/interfaces/email.interface.ts b/packages/twenty-server/src/engine/core-modules/email/interfaces/email.interface.ts deleted file mode 100644 index f359af774..000000000 --- a/packages/twenty-server/src/engine/core-modules/email/interfaces/email.interface.ts +++ /dev/null @@ -1,22 +0,0 @@ -import { FactoryProvider, ModuleMetadata } from '@nestjs/common'; - -import SMTPConnection from 'nodemailer/lib/smtp-connection'; - -export enum EmailDriver { - Logger = 'logger', - Smtp = 'smtp', -} - -export type EmailModuleOptions = - | (SMTPConnection.Options & { - type: EmailDriver.Smtp; - }) - | { - type: EmailDriver.Logger; - }; - -export type EmailModuleAsyncOptions = { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - useFactory: (...args: any[]) => EmailModuleOptions; -} & Pick & - Pick; diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts new file mode 100644 index 000000000..6a6c73c56 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage-driver.factory.spec.ts @@ -0,0 +1,303 @@ +import { Test, TestingModule } from '@nestjs/testing'; + +import { StorageDriverType } from 'src/engine/core-modules/file-storage/interfaces/file-storage.interface'; + +import { FileStorageDriverFactory } from 'src/engine/core-modules/file-storage/file-storage-driver.factory'; +import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; + +describe('FileStorageDriverFactory', () => { + let factory: FileStorageDriverFactory; + let twentyConfigService: TwentyConfigService; + + const mockTwentyConfigService = { + get: jest.fn(), + }; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + FileStorageDriverFactory, + { + provide: TwentyConfigService, + useValue: mockTwentyConfigService, + }, + ], + }).compile(); + + factory = module.get(FileStorageDriverFactory); + twentyConfigService = module.get(TwentyConfigService); + + jest.clearAllMocks(); + }); + + describe('buildConfigKey', () => { + it('should build config key for local storage', () => { + const storagePath = '/tmp/storage'; + + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return storagePath; + + return undefined; + }); + + const result = factory['buildConfigKey'](); + + expect(result).toBe(`local|${storagePath}`); + expect(twentyConfigService.get).toHaveBeenCalledWith('STORAGE_TYPE'); + expect(twentyConfigService.get).toHaveBeenCalledWith( + 'STORAGE_LOCAL_PATH', + ); + }); + + it('should build config key for S3 storage', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue(StorageDriverType.S3); + jest + .spyOn(factory as any, 'getConfigGroupHash') + .mockReturnValue('s3-hash-123'); + + const result = factory['buildConfigKey'](); + + expect(result).toBe('s3|s3-hash-123'); + expect(twentyConfigService.get).toHaveBeenCalledWith('STORAGE_TYPE'); + }); + + it('should throw error for unsupported storage type', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue('unsupported-type'); + + expect(() => factory['buildConfigKey']()).toThrow( + 'Unsupported storage type: unsupported-type', + ); + }); + }); + + describe('createDriver', () => { + it('should create LocalDriver for local storage', () => { + const storagePath = '/tmp/storage'; + + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return storagePath; + + return undefined; + }); + + const driver = factory['createDriver'](); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('LocalDriver'); + }); + + it('should create S3Driver for S3 storage with access keys', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'STORAGE_TYPE': + return StorageDriverType.S3; + case 'STORAGE_S3_NAME': + return 'test-bucket'; + case 'STORAGE_S3_ENDPOINT': + return 'https://s3.amazonaws.com'; + case 'STORAGE_S3_REGION': + return 'us-east-1'; + case 'STORAGE_S3_ACCESS_KEY_ID': + return 'test-access-key'; + case 'STORAGE_S3_SECRET_ACCESS_KEY': + return 'test-secret-key'; + default: + return undefined; + } + }); + + const driver = factory['createDriver'](); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('S3Driver'); + }); + + it('should create S3Driver for S3 storage without access keys (using provider chain)', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'STORAGE_TYPE': + return StorageDriverType.S3; + case 'STORAGE_S3_NAME': + return 'test-bucket'; + case 'STORAGE_S3_ENDPOINT': + return 'https://s3.amazonaws.com'; + case 'STORAGE_S3_REGION': + return 'us-east-1'; + case 'STORAGE_S3_ACCESS_KEY_ID': + return undefined; + case 'STORAGE_S3_SECRET_ACCESS_KEY': + return undefined; + default: + return undefined; + } + }); + + const driver = factory['createDriver'](); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('S3Driver'); + }); + + it('should throw error for invalid storage driver type', () => { + jest.spyOn(twentyConfigService, 'get').mockReturnValue('invalid-type'); + + expect(() => factory['createDriver']()).toThrow( + 'Invalid storage driver type: invalid-type', + ); + }); + }); + + describe('getCurrentDriver', () => { + it('should return current driver for local storage', () => { + const storagePath = '/tmp/storage'; + + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return storagePath; + + return undefined; + }); + + const driver = factory.getCurrentDriver(); + + expect(driver).toBeDefined(); + expect(driver.constructor.name).toBe('LocalDriver'); + }); + + it('should reuse driver when config key unchanged', () => { + const storagePath = '/tmp/storage'; + + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return storagePath; + + return undefined; + }); + + const driver1 = factory.getCurrentDriver(); + const driver2 = factory.getCurrentDriver(); + + expect(driver1).toBe(driver2); + }); + + it('should create new driver when config key changes', () => { + // First call with local storage + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return '/tmp/storage1'; + + return undefined; + }); + + const driver1 = factory.getCurrentDriver(); + + // Second call with different local path + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return '/tmp/storage2'; + + return undefined; + }); + + const driver2 = factory.getCurrentDriver(); + + expect(driver1).not.toBe(driver2); + expect(driver1.constructor.name).toBe('LocalDriver'); + expect(driver2.constructor.name).toBe('LocalDriver'); + }); + + it('should create new driver when switching from local to S3', () => { + // First call with local storage + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return '/tmp/storage'; + + return undefined; + }); + + const driver1 = factory.getCurrentDriver(); + + // Second call with S3 storage + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + switch (key) { + case 'STORAGE_TYPE': + return StorageDriverType.S3; + case 'STORAGE_S3_NAME': + return 'test-bucket'; + case 'STORAGE_S3_ENDPOINT': + return 'https://s3.amazonaws.com'; + case 'STORAGE_S3_REGION': + return 'us-east-1'; + default: + return undefined; + } + }); + jest + .spyOn(factory as any, 'getConfigGroupHash') + .mockReturnValue('s3-hash-123'); + + const driver2 = factory.getCurrentDriver(); + + expect(driver1).not.toBe(driver2); + expect(driver1.constructor.name).toBe('LocalDriver'); + expect(driver2.constructor.name).toBe('S3Driver'); + }); + + it('should throw error for unsupported storage type', () => { + jest + .spyOn(twentyConfigService, 'get') + .mockReturnValue('invalid-storage-type'); + + expect(() => factory.getCurrentDriver()).toThrow( + 'Failed to build config key for FileStorageDriverFactory. Original error: Unsupported storage type: invalid-storage-type', + ); + }); + + it('should throw error when driver creation fails after valid config', () => { + const storagePath = '/tmp/storage'; + + jest + .spyOn(twentyConfigService, 'get') + .mockImplementation((key: string) => { + if (key === 'STORAGE_TYPE') return StorageDriverType.Local; + if (key === 'STORAGE_LOCAL_PATH') return storagePath; + + return undefined; + }); + + jest.spyOn(factory as any, 'createDriver').mockImplementation(() => { + throw new Error('Driver creation failed'); + }); + + expect(() => factory.getCurrentDriver()).toThrow( + 'Failed to create driver for FileStorageDriverFactory with config key: local|/tmp/storage. Original error: Driver creation failed', + ); + }); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts index 21210726e..d3193e01c 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/__tests__/file-storage.service.spec.ts @@ -1,26 +1,317 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { STORAGE_DRIVER } from 'src/engine/core-modules/file-storage/file-storage.constants'; +import { Readable } from 'stream'; + +import { FileStorageDriverFactory } from 'src/engine/core-modules/file-storage/file-storage-driver.factory'; import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; describe('FileStorageService', () => { let service: FileStorageService; + let fileStorageDriverFactory: FileStorageDriverFactory; + + const mockFileStorageDriverFactory = { + getCurrentDriver: jest.fn(), + }; beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ providers: [ FileStorageService, { - provide: STORAGE_DRIVER, - useValue: {}, + provide: FileStorageDriverFactory, + useValue: mockFileStorageDriverFactory, }, ], }).compile(); service = module.get(FileStorageService); + fileStorageDriverFactory = module.get( + FileStorageDriverFactory, + ); + + jest.clearAllMocks(); }); it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('storage operations', () => { + let mockDriver: any; + + beforeEach(() => { + mockDriver = { + write: jest.fn(), + read: jest.fn(), + delete: jest.fn(), + move: jest.fn(), + copy: jest.fn(), + download: jest.fn(), + checkFileExists: jest.fn(), + }; + + mockFileStorageDriverFactory.getCurrentDriver.mockReturnValue(mockDriver); + }); + + describe('write', () => { + it('should delegate to the current driver', async () => { + const writeParams = { + file: Buffer.from('test content'), + name: 'test.txt', + folder: 'documents', + mimeType: 'text/plain', + }; + + mockDriver.write.mockResolvedValue(undefined); + + await service.write(writeParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.write).toHaveBeenCalledWith(writeParams); + }); + + it('should handle write errors', async () => { + const writeParams = { + file: 'test content', + name: 'test.txt', + folder: 'documents', + mimeType: 'text/plain', + }; + + const error = new Error('Write failed'); + + mockDriver.write.mockRejectedValue(error); + + await expect(service.write(writeParams)).rejects.toThrow( + 'Write failed', + ); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.write).toHaveBeenCalledWith(writeParams); + }); + }); + + describe('read', () => { + it('should delegate to the current driver', async () => { + const readParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + const mockStream = new Readable(); + + mockDriver.read.mockResolvedValue(mockStream); + + const result = await service.read(readParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.read).toHaveBeenCalledWith(readParams); + expect(result).toBe(mockStream); + }); + + it('should handle read errors', async () => { + const readParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + const error = new Error('Read failed'); + + mockDriver.read.mockRejectedValue(error); + + await expect(service.read(readParams)).rejects.toThrow('Read failed'); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.read).toHaveBeenCalledWith(readParams); + }); + }); + + describe('delete', () => { + it('should delegate to the current driver with filename', async () => { + const deleteParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + mockDriver.delete.mockResolvedValue(undefined); + + await service.delete(deleteParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.delete).toHaveBeenCalledWith(deleteParams); + }); + + it('should delegate to the current driver without filename (delete folder)', async () => { + const deleteParams = { + folderPath: 'documents', + }; + + mockDriver.delete.mockResolvedValue(undefined); + + await service.delete(deleteParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.delete).toHaveBeenCalledWith(deleteParams); + }); + + it('should handle delete errors', async () => { + const deleteParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + const error = new Error('Delete failed'); + + mockDriver.delete.mockRejectedValue(error); + + await expect(service.delete(deleteParams)).rejects.toThrow( + 'Delete failed', + ); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.delete).toHaveBeenCalledWith(deleteParams); + }); + }); + + describe('move', () => { + it('should delegate to the current driver', async () => { + const moveParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: 'archive', filename: 'archived-test.txt' }, + }; + + mockDriver.move.mockResolvedValue(undefined); + + await service.move(moveParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.move).toHaveBeenCalledWith(moveParams); + }); + + it('should handle move errors', async () => { + const moveParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: 'archive', filename: 'archived-test.txt' }, + }; + + const error = new Error('Move failed'); + + mockDriver.move.mockRejectedValue(error); + + await expect(service.move(moveParams)).rejects.toThrow('Move failed'); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.move).toHaveBeenCalledWith(moveParams); + }); + }); + + describe('copy', () => { + it('should delegate to the current driver', async () => { + const copyParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: 'backup', filename: 'test-backup.txt' }, + }; + + mockDriver.copy.mockResolvedValue(undefined); + + await service.copy(copyParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.copy).toHaveBeenCalledWith(copyParams); + }); + + it('should handle copy errors', async () => { + const copyParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: 'backup', filename: 'test-backup.txt' }, + }; + + const error = new Error('Copy failed'); + + mockDriver.copy.mockRejectedValue(error); + + await expect(service.copy(copyParams)).rejects.toThrow('Copy failed'); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.copy).toHaveBeenCalledWith(copyParams); + }); + }); + + describe('download', () => { + it('should delegate to the current driver', async () => { + const downloadParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: '/tmp', filename: 'downloaded-test.txt' }, + }; + + mockDriver.download.mockResolvedValue(undefined); + + await service.download(downloadParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.download).toHaveBeenCalledWith(downloadParams); + }); + + it('should handle download errors', async () => { + const downloadParams = { + from: { folderPath: 'documents', filename: 'test.txt' }, + to: { folderPath: '/tmp', filename: 'downloaded-test.txt' }, + }; + + const error = new Error('Download failed'); + + mockDriver.download.mockRejectedValue(error); + + await expect(service.download(downloadParams)).rejects.toThrow( + 'Download failed', + ); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.download).toHaveBeenCalledWith(downloadParams); + }); + }); + + describe('checkFileExists', () => { + it('should delegate to the current driver and return true', async () => { + const checkParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + mockDriver.checkFileExists.mockResolvedValue(true); + + const result = await service.checkFileExists(checkParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); + expect(result).toBe(true); + }); + + it('should delegate to the current driver and return false', async () => { + const checkParams = { + folderPath: 'documents', + filename: 'nonexistent.txt', + }; + + mockDriver.checkFileExists.mockResolvedValue(false); + + const result = await service.checkFileExists(checkParams); + + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); + expect(result).toBe(false); + }); + + it('should handle checkFileExists errors', async () => { + const checkParams = { + folderPath: 'documents', + filename: 'test.txt', + }; + + const error = new Error('Check failed'); + + mockDriver.checkFileExists.mockRejectedValue(error); + + await expect(service.checkFileExists(checkParams)).rejects.toThrow( + 'Check failed', + ); + expect(fileStorageDriverFactory.getCurrentDriver).toHaveBeenCalled(); + expect(mockDriver.checkFileExists).toHaveBeenCalledWith(checkParams); + }); + }); + }); }); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts new file mode 100644 index 000000000..c5e323bce --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage-driver.factory.ts @@ -0,0 +1,79 @@ +import { Injectable } from '@nestjs/common'; + +import { fromNodeProviderChain } from '@aws-sdk/credential-providers'; + +import { StorageDriver } from 'src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface'; +import { StorageDriverType } from 'src/engine/core-modules/file-storage/interfaces/file-storage.interface'; + +import { LocalDriver } from 'src/engine/core-modules/file-storage/drivers/local.driver'; +import { S3Driver } from 'src/engine/core-modules/file-storage/drivers/s3.driver'; +import { DriverFactoryBase } from 'src/engine/core-modules/twenty-config/dynamic-factory.base'; +import { ConfigVariablesGroup } from 'src/engine/core-modules/twenty-config/enums/config-variables-group.enum'; +import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; +import { resolveAbsolutePath } from 'src/utils/resolve-absolute-path'; + +@Injectable() +export class FileStorageDriverFactory extends DriverFactoryBase { + constructor(twentyConfigService: TwentyConfigService) { + super(twentyConfigService); + } + + protected buildConfigKey(): string { + const storageType = this.twentyConfigService.get('STORAGE_TYPE'); + + if (storageType === StorageDriverType.Local) { + const storagePath = this.twentyConfigService.get('STORAGE_LOCAL_PATH'); + + return `local|${storagePath}`; + } + + if (storageType === StorageDriverType.S3) { + const storageConfigHash = this.getConfigGroupHash( + ConfigVariablesGroup.StorageConfig, + ); + + return `s3|${storageConfigHash}`; + } + + throw new Error(`Unsupported storage type: ${storageType}`); + } + + protected createDriver(): StorageDriver { + const storageType = this.twentyConfigService.get('STORAGE_TYPE'); + + switch (storageType) { + case StorageDriverType.Local: { + const storagePath = this.twentyConfigService.get('STORAGE_LOCAL_PATH'); + + return new LocalDriver({ + storagePath: resolveAbsolutePath(storagePath), + }); + } + + case StorageDriverType.S3: { + const bucketName = this.twentyConfigService.get('STORAGE_S3_NAME'); + const endpoint = this.twentyConfigService.get('STORAGE_S3_ENDPOINT'); + const region = this.twentyConfigService.get('STORAGE_S3_REGION'); + const accessKeyId = this.twentyConfigService.get( + 'STORAGE_S3_ACCESS_KEY_ID', + ); + const secretAccessKey = this.twentyConfigService.get( + 'STORAGE_S3_SECRET_ACCESS_KEY', + ); + + return new S3Driver({ + bucketName: bucketName ?? '', + endpoint: endpoint, + credentials: accessKeyId + ? { accessKeyId, secretAccessKey } + : fromNodeProviderChain({ clientConfig: { region } }), + forcePathStyle: true, + region: region ?? '', + }); + } + + default: + throw new Error(`Invalid storage driver type: ${storageType}`); + } + } +} diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.constants.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.constants.ts deleted file mode 100644 index 1c726adbc..000000000 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.constants.ts +++ /dev/null @@ -1 +0,0 @@ -export const STORAGE_DRIVER = Symbol('STORAGE_DRIVER'); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-definition.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-definition.ts deleted file mode 100644 index 4d453abf4..000000000 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-definition.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { ConfigurableModuleBuilder } from '@nestjs/common'; - -import { FileStorageModuleOptions } from 'src/engine/core-modules/file-storage/interfaces'; - -export const { - ConfigurableModuleClass, - MODULE_OPTIONS_TOKEN, - OPTIONS_TYPE, - ASYNC_OPTIONS_TYPE, -} = new ConfigurableModuleBuilder({ - moduleName: 'FileStorage', -}) - .setClassMethodName('forRoot') - .build(); diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-factory.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-factory.ts deleted file mode 100644 index edb23a2f2..000000000 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module-factory.ts +++ /dev/null @@ -1,63 +0,0 @@ -import { fromNodeProviderChain } from '@aws-sdk/credential-providers'; - -import { - FileStorageModuleOptions, - StorageDriverType, -} from 'src/engine/core-modules/file-storage/interfaces'; -import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; -import { resolveAbsolutePath } from 'src/utils/resolve-absolute-path'; - -/** - * FileStorage Module factory - * @returns FileStorageModuleOptions - * @param twentyConfigService - */ -export const fileStorageModuleFactory = async ( - twentyConfigService: TwentyConfigService, -): Promise => { - const driverType = twentyConfigService.get('STORAGE_TYPE'); - - switch (driverType) { - case StorageDriverType.Local: { - const storagePath = twentyConfigService.get('STORAGE_LOCAL_PATH'); - - return { - type: StorageDriverType.Local, - options: { - storagePath: resolveAbsolutePath(storagePath), - }, - }; - } - case StorageDriverType.S3: { - const bucketName = twentyConfigService.get('STORAGE_S3_NAME'); - const endpoint = twentyConfigService.get('STORAGE_S3_ENDPOINT'); - const region = twentyConfigService.get('STORAGE_S3_REGION'); - const accessKeyId = twentyConfigService.get('STORAGE_S3_ACCESS_KEY_ID'); - const secretAccessKey = twentyConfigService.get( - 'STORAGE_S3_SECRET_ACCESS_KEY', - ); - - return { - type: StorageDriverType.S3, - options: { - bucketName: bucketName ?? '', - endpoint: endpoint, - credentials: accessKeyId - ? { - accessKeyId, - secretAccessKey, - } - : fromNodeProviderChain({ - clientConfig: { region }, - }), - forcePathStyle: true, - region: region ?? '', - }, - }; - } - default: - throw new Error( - `Invalid storage driver type (${driverType}), check your .env file`, - ); - } -}; diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module.ts index 44fc5990b..2b1afe7cc 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.module.ts @@ -1,50 +1,16 @@ import { DynamicModule, Global } from '@nestjs/common'; +import { FileStorageDriverFactory } from 'src/engine/core-modules/file-storage/file-storage-driver.factory'; import { FileStorageService } from 'src/engine/core-modules/file-storage/file-storage.service'; -import { - FileStorageModuleAsyncOptions, - FileStorageModuleOptions, -} from 'src/engine/core-modules/file-storage/interfaces'; -import { STORAGE_DRIVER } from 'src/engine/core-modules/file-storage/file-storage.constants'; -import { LocalDriver } from 'src/engine/core-modules/file-storage/drivers/local.driver'; -import { S3Driver } from 'src/engine/core-modules/file-storage/drivers/s3.driver'; +import { TwentyConfigModule } from 'src/engine/core-modules/twenty-config/twenty-config.module'; @Global() export class FileStorageModule { - static forRoot(options: FileStorageModuleOptions): DynamicModule { - const provider = { - provide: STORAGE_DRIVER, - useValue: - options.type === 's3' - ? new S3Driver(options.options) - : new LocalDriver(options.options), - }; - + static forRoot(): DynamicModule { return { module: FileStorageModule, - providers: [FileStorageService, provider], - exports: [FileStorageService], - }; - } - - static forRootAsync(options: FileStorageModuleAsyncOptions): DynamicModule { - const provider = { - provide: STORAGE_DRIVER, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - useFactory: async (...args: any[]) => { - const config = await options.useFactory(...args); - - return config?.type === 's3' - ? new S3Driver(config.options) - : new LocalDriver(config.options); - }, - inject: options.inject || [], - }; - - return { - module: FileStorageModule, - imports: options.imports || [], - providers: [FileStorageService, provider], + imports: [TwentyConfigModule], + providers: [FileStorageDriverFactory, FileStorageService], exports: [FileStorageService], }; } diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts index 204701196..ace0d2c7a 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/file-storage.service.ts @@ -1,18 +1,16 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { Readable } from 'stream'; import { StorageDriver } from 'src/engine/core-modules/file-storage/drivers/interfaces/storage-driver.interface'; -import { STORAGE_DRIVER } from 'src/engine/core-modules/file-storage/file-storage.constants'; +import { FileStorageDriverFactory } from 'src/engine/core-modules/file-storage/file-storage-driver.factory'; @Injectable() export class FileStorageService implements StorageDriver { - constructor(@Inject(STORAGE_DRIVER) private driver: StorageDriver) {} - - delete(params: { folderPath: string; filename?: string }): Promise { - return this.driver.delete(params); - } + constructor( + private readonly fileStorageDriverFactory: FileStorageDriverFactory, + ) {} write(params: { file: string | Buffer | Uint8Array; @@ -20,38 +18,56 @@ export class FileStorageService implements StorageDriver { folder: string; mimeType: string | undefined; }): Promise { - return this.driver.write(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.write(params); } read(params: { folderPath: string; filename: string }): Promise { - return this.driver.read(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.read(params); + } + + delete(params: { folderPath: string; filename?: string }): Promise { + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.delete(params); } move(params: { from: { folderPath: string; filename: string }; to: { folderPath: string; filename: string }; }): Promise { - return this.driver.move(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.move(params); } copy(params: { from: { folderPath: string; filename?: string }; to: { folderPath: string; filename?: string }; }): Promise { - return this.driver.copy(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.copy(params); } download(params: { from: { folderPath: string; filename?: string }; to: { folderPath: string; filename?: string }; }): Promise { - return this.driver.download(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.download(params); } checkFileExists(params: { folderPath: string; filename: string; }): Promise { - return this.driver.checkFileExists(params); + const driver = this.fileStorageDriverFactory.getCurrentDriver(); + + return driver.checkFileExists(params); } } diff --git a/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage.interface.ts b/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage.interface.ts index bfac55fbb..f36162f30 100644 --- a/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage.interface.ts +++ b/packages/twenty-server/src/engine/core-modules/file-storage/interfaces/file-storage.interface.ts @@ -1,31 +1,4 @@ -import { FactoryProvider, ModuleMetadata } from '@nestjs/common'; - -import { S3DriverOptions } from 'src/engine/core-modules/file-storage/drivers/s3.driver'; -import { LocalDriverOptions } from 'src/engine/core-modules/file-storage/drivers/local.driver'; - export enum StorageDriverType { S3 = 's3', Local = 'local', } - -export interface S3DriverFactoryOptions { - type: StorageDriverType.S3; - options: S3DriverOptions; -} - -export interface LocalDriverFactoryOptions { - type: StorageDriverType.Local; - options: LocalDriverOptions; -} - -export type FileStorageModuleOptions = - | S3DriverFactoryOptions - | LocalDriverFactoryOptions; - -export type FileStorageModuleAsyncOptions = { - useFactory: ( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ...args: any[] - ) => FileStorageModuleOptions | Promise; -} & Pick & - Pick; diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/cache/__tests__/config-cache.service.spec.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/cache/__tests__/config-cache.service.spec.ts index 33a219a80..28787a297 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/cache/__tests__/config-cache.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/cache/__tests__/config-cache.service.spec.ts @@ -44,15 +44,15 @@ describe('ConfigCacheService', () => { it('should handle different value types', () => { const booleanKey = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; const stringKey = 'EMAIL_FROM_ADDRESS' as keyof ConfigVariables; - const numberKey = 'NODE_PORT' as keyof ConfigVariables; + const numberKey = 'CACHE_STORAGE_TTL' as keyof ConfigVariables; service.set(booleanKey, true); service.set(stringKey, 'test@example.com'); - service.set(numberKey, 3000); + service.set(numberKey, 3600 * 24 * 7); expect(service.get(booleanKey)).toBe(true); expect(service.get(stringKey)).toBe('test@example.com'); - expect(service.get(numberKey)).toBe(3000); + expect(service.get(numberKey)).toBe(3600 * 24 * 7); }); }); @@ -105,7 +105,7 @@ describe('ConfigCacheService', () => { it('should return correct cache information', () => { const key1 = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; const key2 = 'EMAIL_FROM_ADDRESS' as keyof ConfigVariables; - const key3 = 'NODE_PORT' as keyof ConfigVariables; + const key3 = 'CACHE_STORAGE_TTL' as keyof ConfigVariables; service.set(key1, true); service.set(key2, 'test@example.com'); diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts index 0cf26149d..9fe018bd6 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/config-variables.ts @@ -11,7 +11,6 @@ import { } from 'class-validator'; import { isDefined } from 'twenty-shared/utils'; -import { EmailDriver } from 'src/engine/core-modules/email/interfaces/email.interface'; import { LLMChatModelDriver } from 'src/engine/core-modules/llm-chat-model/interfaces/llm-chat-model.interface'; import { LLMTracingDriver } from 'src/engine/core-modules/llm-tracing/interfaces/llm-tracing.interface'; import { AwsRegion } from 'src/engine/core-modules/twenty-config/interfaces/aws-region.interface'; @@ -19,6 +18,7 @@ import { NodeEnvironment } from 'src/engine/core-modules/twenty-config/interface import { SupportDriver } from 'src/engine/core-modules/twenty-config/interfaces/support.interface'; import { CaptchaDriverType } from 'src/engine/core-modules/captcha/interfaces'; +import { EmailDriver } from 'src/engine/core-modules/email/enums/email-driver.enum'; import { ExceptionHandlerDriver } from 'src/engine/core-modules/exception-handler/interfaces'; import { StorageDriverType } from 'src/engine/core-modules/file-storage/interfaces'; import { LoggerDriverType } from 'src/engine/core-modules/logger/interfaces'; @@ -209,6 +209,7 @@ export class ConfigVariables { description: 'Legacy variable to be deprecated when all API Keys expire. Replaced by APP_KEY', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsOptional() ACCESS_TOKEN_SECRET: string; @@ -411,6 +412,7 @@ export class ConfigVariables { description: 'Type of serverless execution (local or Lambda)', type: ConfigVariableType.ENUM, options: Object.values(ServerlessDriverType), + isEnvOnly: true, }) @IsOptional() SERVERLESS_TYPE: ServerlessDriverType = ServerlessDriverType.Local; @@ -593,6 +595,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.ServerConfig, description: 'Url for the frontend application', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsUrl({ require_tld: false, require_protocol: true }) @IsOptional() @@ -628,6 +631,7 @@ export class ConfigVariables { description: 'Driver used for handling exceptions (Console or Sentry)', type: ConfigVariableType.ENUM, options: Object.values(ExceptionHandlerDriver), + isEnvOnly: true, }) @IsOptional() EXCEPTION_HANDLER_DRIVER: ExceptionHandlerDriver = @@ -637,7 +641,8 @@ export class ConfigVariables { group: ConfigVariablesGroup.Logging, description: 'Levels of logging to be captured', type: ConfigVariableType.ARRAY, - options: ['log', 'error', 'warn', 'debug', 'verbose'], + options: ['log', 'error', 'warn', 'debug'], + isEnvOnly: true, }) @CastToLogLevelArray() @IsOptional() @@ -648,6 +653,7 @@ export class ConfigVariables { description: 'Driver used for collect metrics (OpenTelemetry or Console)', type: ConfigVariableType.ARRAY, options: ['OpenTelemetry', 'Console'], + isEnvOnly: true, }) @CastToMeterDriverArray() @IsOptional() @@ -657,6 +663,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.Metering, description: 'Endpoint URL for the OpenTelemetry collector', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsOptional() OTLP_COLLECTOR_ENDPOINT_URL: string; @@ -666,6 +673,7 @@ export class ConfigVariables { description: 'Driver used for logging (only console for now)', type: ConfigVariableType.ENUM, options: Object.values(LoggerDriverType), + isEnvOnly: true, }) @IsOptional() LOGGER_DRIVER: LoggerDriverType = LoggerDriverType.Console; @@ -830,6 +838,7 @@ export class ConfigVariables { description: 'Node environment (development, production, etc.)', type: ConfigVariableType.ENUM, options: Object.values(NodeEnvironment), + isEnvOnly: true, }) NODE_ENV: NodeEnvironment = NodeEnvironment.production; @@ -837,6 +846,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.ServerConfig, description: 'Port for the node server', type: ConfigVariableType.NUMBER, + isEnvOnly: true, }) @CastToPositiveNumber() @IsOptional() @@ -846,6 +856,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.ServerConfig, description: 'Base URL for the server', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsUrl({ require_tld: false, require_protocol: true }) @IsOptional() @@ -890,6 +901,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.SSL, description: 'Path to the SSL key for enabling HTTPS in local development', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsOptional() SSL_KEY_PATH: string; @@ -899,6 +911,7 @@ export class ConfigVariables { description: 'Path to the SSL certificate for enabling HTTPS in local development', type: ConfigVariableType.STRING, + isEnvOnly: true, }) @IsOptional() SSL_CERT_PATH: string; @@ -934,6 +947,7 @@ export class ConfigVariables { description: 'Driver for the LLM chat model', type: ConfigVariableType.ENUM, options: Object.values(LLMChatModelDriver), + isEnvOnly: true, }) LLM_CHAT_MODEL_DRIVER: LLMChatModelDriver; @@ -965,6 +979,7 @@ export class ConfigVariables { description: 'Driver for LLM tracing', type: ConfigVariableType.ENUM, options: Object.values(LLMTracingDriver), + isEnvOnly: true, }) LLM_TRACING_DRIVER: LLMTracingDriver = LLMTracingDriver.Console; @@ -972,6 +987,7 @@ export class ConfigVariables { group: ConfigVariablesGroup.ServerConfig, description: 'Enable or disable multi-workspace support', type: ConfigVariableType.BOOLEAN, + isEnvOnly: true, }) @IsOptional() IS_MULTIWORKSPACE_ENABLED = false; @@ -1040,6 +1056,7 @@ export class ConfigVariables { description: 'Driver for captcha integration', type: ConfigVariableType.ENUM, options: Object.values(CaptchaDriverType), + isEnvOnly: true, }) @IsOptional() CAPTCHA_DRIVER?: CaptchaDriverType; diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/conversion/__tests__/config-value-converter.service.spec.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/conversion/__tests__/config-value-converter.service.spec.ts index b7179bf55..c2800b14b 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/conversion/__tests__/config-value-converter.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/conversion/__tests__/config-value-converter.service.spec.ts @@ -78,7 +78,7 @@ describe('ConfigValueConverterService', () => { }); const mockConfigVariables = { - NODE_PORT: 3000, + CACHE_STORAGE_TTL: 3600 * 24 * 7, }; const module: TestingModule = await Test.createTestingModule({ @@ -122,25 +122,25 @@ describe('ConfigValueConverterService', () => { it('should use number transformer for number type', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce({ - NODE_PORT: { + CACHE_STORAGE_TTL: { type: ConfigVariableType.NUMBER, group: ConfigVariablesGroup.ServerConfig, - description: 'Port for the node server', + description: 'Time-to-live for cache storage in seconds', }, }); - typeTransformers.number.toApp.mockReturnValueOnce(3000); + typeTransformers.number.toApp.mockReturnValueOnce(3600 * 24 * 7); const result = service.convertDbValueToAppValue( - '3000', - 'NODE_PORT' as keyof ConfigVariables, + '604800', + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(typeTransformers.number.toApp).toHaveBeenCalledWith( - '3000', + '604800', undefined, ); - expect(result).toBe(3000); + expect(result).toBe(3600 * 24 * 7); }); it('should use string transformer for string type', () => { @@ -222,28 +222,28 @@ describe('ConfigValueConverterService', () => { it('should infer type from default value when no metadata is available', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce(undefined); - typeTransformers.number.toApp.mockReturnValueOnce(3000); + typeTransformers.number.toApp.mockReturnValueOnce(3600 * 24 * 7); const result = service.convertDbValueToAppValue( - '3000', - 'NODE_PORT' as keyof ConfigVariables, + '604800', + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(typeTransformers.number.toApp).toHaveBeenCalledWith( - '3000', + '604800', undefined, ); - expect(result).toBe(3000); + expect(result).toBe(3600 * 24 * 7); }); it('should return undefined for null or undefined input', () => { const result1 = service.convertDbValueToAppValue( null, - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); const result2 = service.convertDbValueToAppValue( undefined, - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(result1).toBeUndefined(); @@ -252,10 +252,10 @@ describe('ConfigValueConverterService', () => { it('should propagate errors from transformers with context', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce({ - NODE_PORT: { + CACHE_STORAGE_TTL: { type: ConfigVariableType.NUMBER, group: ConfigVariablesGroup.ServerConfig, - description: 'Port for the node server', + description: 'Time-to-live for cache storage in seconds', }, }); @@ -268,9 +268,11 @@ describe('ConfigValueConverterService', () => { expect(() => { service.convertDbValueToAppValue( 'not-a-number', - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); - }).toThrow(`Failed to convert NODE_PORT to app value: Test error`); + }).toThrow( + `Failed to convert CACHE_STORAGE_TTL to app value: Test error`, + ); }); }); @@ -300,25 +302,25 @@ describe('ConfigValueConverterService', () => { it('should use number transformer for number type', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce({ - NODE_PORT: { + CACHE_STORAGE_TTL: { type: ConfigVariableType.NUMBER, group: ConfigVariablesGroup.ServerConfig, - description: 'Port for the node server', + description: 'Time-to-live for cache storage in seconds', }, }); - typeTransformers.number.toStorage.mockReturnValueOnce(3000); + typeTransformers.number.toStorage.mockReturnValueOnce(3600 * 24 * 7); const result = service.convertAppValueToDbValue( - 3000, - 'NODE_PORT' as keyof ConfigVariables, + 3600 * 24 * 7, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(typeTransformers.number.toStorage).toHaveBeenCalledWith( - 3000, + 3600 * 24 * 7, undefined, ); - expect(result).toBe(3000); + expect(result).toBe(3600 * 24 * 7); }); it('should use string transformer for string type', () => { @@ -400,28 +402,28 @@ describe('ConfigValueConverterService', () => { it('should infer type from default value when no metadata is available', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce(undefined); - typeTransformers.number.toStorage.mockReturnValueOnce(3000); + typeTransformers.number.toStorage.mockReturnValueOnce(3600 * 24 * 7); const result = service.convertAppValueToDbValue( - 3000, - 'NODE_PORT' as keyof ConfigVariables, + 3600 * 24 * 7, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(typeTransformers.number.toStorage).toHaveBeenCalledWith( - 3000, + 3600 * 24 * 7, undefined, ); - expect(result).toBe(3000); + expect(result).toBe(3600 * 24 * 7); }); it('should return null for null or undefined input', () => { const result1 = service.convertAppValueToDbValue( null, - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); const result2 = service.convertAppValueToDbValue( undefined, - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); expect(result1).toBeNull(); @@ -449,10 +451,10 @@ describe('ConfigValueConverterService', () => { it('should propagate errors from transformers with context', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce({ - NODE_PORT: { + CACHE_STORAGE_TTL: { type: ConfigVariableType.NUMBER, group: ConfigVariablesGroup.ServerConfig, - description: 'Port for the node server', + description: 'Time-to-live for cache storage in seconds', }, }); @@ -465,9 +467,9 @@ describe('ConfigValueConverterService', () => { expect(() => { service.convertAppValueToDbValue( 'not-a-number' as any, - 'NODE_PORT' as keyof ConfigVariables, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); - }).toThrow(`Failed to convert NODE_PORT to DB value: Test error`); + }).toThrow(`Failed to convert CACHE_STORAGE_TTL to DB value: Test error`); }); }); @@ -497,10 +499,10 @@ describe('ConfigValueConverterService', () => { it('should validate number values when converting to storage', () => { jest.spyOn(TypedReflect, 'getMetadata').mockReturnValueOnce({ - NODE_PORT: { + CACHE_STORAGE_TTL: { type: ConfigVariableType.NUMBER, group: ConfigVariablesGroup.ServerConfig, - description: 'Port for the node server', + description: 'Time-to-live for cache storage in seconds', }, }); @@ -510,11 +512,11 @@ describe('ConfigValueConverterService', () => { expect(() => { service.convertAppValueToDbValue( - 'invalid-port' as any, - 'NODE_PORT' as keyof ConfigVariables, + 'invalid-ttl' as any, + 'CACHE_STORAGE_TTL' as keyof ConfigVariables, ); }).toThrow( - 'Failed to convert NODE_PORT to DB value: Expected number, got string', + 'Failed to convert CACHE_STORAGE_TTL to DB value: Expected number, got string', ); }); diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/dynamic-factory.base.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/dynamic-factory.base.ts new file mode 100644 index 000000000..f03fabec3 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/dynamic-factory.base.ts @@ -0,0 +1,73 @@ +import { createHash } from 'crypto'; + +import { ConfigVariables } from 'src/engine/core-modules/twenty-config/config-variables'; +import { ConfigVariablesGroup } from 'src/engine/core-modules/twenty-config/enums/config-variables-group.enum'; +import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; +import { TypedReflect } from 'src/utils/typed-reflect'; + +export abstract class DriverFactoryBase { + private currentDriver: TDriver | null = null; + private currentConfigKey: string | null = null; + + constructor(protected readonly twentyConfigService: TwentyConfigService) {} + + getCurrentDriver(): TDriver { + let configKey: string; + + try { + configKey = this.buildConfigKey(); + } catch (error) { + throw new Error( + `Failed to build config key for ${this.constructor.name}. Original error: ${error instanceof Error ? error.message : String(error)}`, + ); + } + + if (this.currentConfigKey !== configKey) { + try { + this.currentDriver = this.createDriver(); + } catch (error) { + throw new Error( + `Failed to create driver for ${this.constructor.name} with config key: ${configKey}. Original error: ${error instanceof Error ? error.message : String(error)}`, + ); + } + + this.currentConfigKey = configKey; + } + + if (!this.currentDriver) { + throw new Error( + `Failed to create driver for ${this.constructor.name} with config key: ${configKey}`, + ); + } + + return this.currentDriver; + } + + protected getConfigGroupHash(group: ConfigVariablesGroup): string { + const groupVariables = this.getConfigVariablesByGroup(group); + + const configValues = groupVariables + .map((key) => `${key}=${this.twentyConfigService.get(key)}`) + .sort() + .join('|'); + + return createHash('sha256') + .update(configValues) + .digest('hex') + .substring(0, 16); + } + + private getConfigVariablesByGroup( + group: ConfigVariablesGroup, + ): Array { + const metadata = + TypedReflect.getMetadata('config-variables', ConfigVariables) ?? {}; + + return Object.keys(metadata) + .filter((key) => metadata[key]?.group === group) + .map((key) => key as keyof ConfigVariables); + } + + protected abstract buildConfigKey(): string; + protected abstract createDriver(): TDriver; +} diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts index 75de6fea6..16e9ebfa1 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/storage/__tests__/config-storage.service.spec.ts @@ -14,6 +14,10 @@ import { EnvironmentConfigDriver } from 'src/engine/core-modules/twenty-config/d import { ConfigVariableType } from 'src/engine/core-modules/twenty-config/enums/config-variable-type.enum'; import { ConfigVariablesGroup } from 'src/engine/core-modules/twenty-config/enums/config-variables-group.enum'; import { ConfigStorageService } from 'src/engine/core-modules/twenty-config/storage/config-storage.service'; +import { + ConfigVariableException, + ConfigVariableExceptionCode, +} from 'src/engine/core-modules/twenty-config/twenty-config.exception'; import { User } from 'src/engine/core-modules/user/user.entity'; import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity'; import { TypedReflect } from 'src/utils/typed-reflect'; @@ -154,7 +158,10 @@ describe('ConfigStorageService', () => { throw error; }); - await expect(service.get(key)).rejects.toThrow('Conversion error'); + await expect(service.get(key)).rejects.toThrow(ConfigVariableException); + await expect(service.get(key)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.VALIDATION_FAILED, + }); }); it('should decrypt sensitive string values', async () => { @@ -250,93 +257,26 @@ describe('ConfigStorageService', () => { const result = await service.get(key); - expect(result).toBe(encryptedValue); // Should fall back to encrypted value - expect(environmentConfigDriver.get).toHaveBeenCalledWith('APP_SECRET'); + expect(result).toBe(encryptedValue); + expect(authUtils.decryptText).toHaveBeenCalledWith( + encryptedValue, + 'test-secret', + ); }); - it('should skip decryption for non-string sensitive values', async () => { - const key = 'SENSITIVE_CONFIG' as keyof ConfigVariables; - const value = { someKey: 'someValue' }; + it('should handle findOne errors', async () => { + const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; + const error = new Error('Database error'); - const mockRecord = createMockKeyValuePair( - key as string, - JSON.stringify(value), + jest.spyOn(keyValuePairRepository, 'findOne').mockRejectedValue(error); + + await expect(service.get(key)).rejects.toThrow(ConfigVariableException); + await expect(service.get(key)).rejects.toThrow( + `Failed to retrieve config variable ${key}: Database error`, ); - - jest - .spyOn(keyValuePairRepository, 'findOne') - .mockResolvedValue(mockRecord); - - jest.spyOn(TypedReflect, 'getMetadata').mockReturnValue({ - [key]: { - isSensitive: true, - type: ConfigVariableType.ARRAY, - group: ConfigVariablesGroup.ServerConfig, - description: 'Test sensitive config', - }, + await expect(service.get(key)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.INTERNAL_ERROR, }); - - ( - configValueConverter.convertDbValueToAppValue as jest.Mock - ).mockReturnValue(value); - - const result = await service.get(key); - - expect(result).toBe(value); - expect(authUtils.decryptText).not.toHaveBeenCalled(); - }); - - it('should handle decryption failure in loadAll() by skipping failed values', async () => { - const configVars: KeyValuePair[] = [ - createMockKeyValuePair('SENSITIVE_CONFIG_1', 'encrypted:value1'), - createMockKeyValuePair('SENSITIVE_CONFIG_2', 'encrypted:value2'), - createMockKeyValuePair('NORMAL_CONFIG', 'normal-value'), - ]; - - jest.spyOn(keyValuePairRepository, 'find').mockResolvedValue(configVars); - jest.spyOn(TypedReflect, 'getMetadata').mockReturnValue({ - SENSITIVE_CONFIG_1: { - isSensitive: true, - type: ConfigVariableType.STRING, - group: ConfigVariablesGroup.ServerConfig, - description: 'Test sensitive config 1', - }, - SENSITIVE_CONFIG_2: { - isSensitive: true, - type: ConfigVariableType.STRING, - group: ConfigVariablesGroup.ServerConfig, - description: 'Test sensitive config 2', - }, - NORMAL_CONFIG: { - type: ConfigVariableType.STRING, - group: ConfigVariablesGroup.ServerConfig, - description: 'Test normal config', - }, - }); - - ( - configValueConverter.convertDbValueToAppValue as jest.Mock - ).mockImplementation((value) => value); - - // Mock decryption to fail for the second sensitive value - (authUtils.decryptText as jest.Mock) - .mockImplementationOnce((text) => text.replace('encrypted:', '')) - .mockImplementationOnce(() => { - throw new Error('Decryption failed'); - }); - - const result = await service.loadAll(); - - expect(result.size).toBe(3); - expect(result.get('SENSITIVE_CONFIG_1' as keyof ConfigVariables)).toBe( - 'value1', - ); - expect(result.get('SENSITIVE_CONFIG_2' as keyof ConfigVariables)).toBe( - 'encrypted:value2', - ); // Original encrypted value - expect(result.get('NORMAL_CONFIG' as keyof ConfigVariables)).toBe( - 'normal-value', - ); }); }); @@ -344,7 +284,7 @@ describe('ConfigStorageService', () => { it('should update existing record', async () => { const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; const value = true; - const storedValue = 'true'; + const convertedValue = 'true'; const mockRecord = createMockKeyValuePair(key as string, 'false'); @@ -354,32 +294,32 @@ describe('ConfigStorageService', () => { ( configValueConverter.convertAppValueToDbValue as jest.Mock - ).mockReturnValue(storedValue); + ).mockReturnValue(convertedValue); await service.set(key, value); expect(keyValuePairRepository.update).toHaveBeenCalledWith( - { id: '1' }, - { value: storedValue }, + { id: mockRecord.id }, + { value: convertedValue }, ); }); - it('should insert new record', async () => { + it('should insert new record when not found', async () => { const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; const value = true; - const storedValue = 'true'; + const convertedValue = 'true'; jest.spyOn(keyValuePairRepository, 'findOne').mockResolvedValue(null); ( configValueConverter.convertAppValueToDbValue as jest.Mock - ).mockReturnValue(storedValue); + ).mockReturnValue(convertedValue); await service.set(key, value); expect(keyValuePairRepository.insert).toHaveBeenCalledWith({ key: key as string, - value: storedValue, + value: convertedValue, userId: null, workspaceId: null, type: KeyValuePairType.CONFIG_VARIABLE, @@ -388,8 +328,8 @@ describe('ConfigStorageService', () => { it('should handle conversion errors', async () => { const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; - const value = true; - const error = new Error('Conversion error'); + const value = 'not-a-boolean'; + const error = new Error('Invalid boolean value'); ( configValueConverter.convertAppValueToDbValue as jest.Mock @@ -397,7 +337,68 @@ describe('ConfigStorageService', () => { throw error; }); - await expect(service.set(key, value)).rejects.toThrow('Conversion error'); + await expect( + service.set(key, value as unknown as boolean), + ).rejects.toThrow(ConfigVariableException); + await expect( + service.set(key, value as unknown as boolean), + ).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.VALIDATION_FAILED, + }); + }); + + it('should handle update errors', async () => { + const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; + const value = true; + const convertedValue = 'true'; + const error = new Error('Update error'); + + const mockRecord = createMockKeyValuePair(key as string, 'false'); + + jest + .spyOn(keyValuePairRepository, 'findOne') + .mockResolvedValue(mockRecord); + + ( + configValueConverter.convertAppValueToDbValue as jest.Mock + ).mockReturnValue(convertedValue); + + jest.spyOn(keyValuePairRepository, 'update').mockRejectedValue(error); + + await expect(service.set(key, value)).rejects.toThrow( + ConfigVariableException, + ); + await expect(service.set(key, value)).rejects.toThrow( + `Failed to save config variable ${key}: Update error`, + ); + await expect(service.set(key, value)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.INTERNAL_ERROR, + }); + }); + + it('should handle insert errors', async () => { + const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; + const value = true; + const convertedValue = 'true'; + const error = new Error('Insert error'); + + jest.spyOn(keyValuePairRepository, 'findOne').mockResolvedValue(null); + + ( + configValueConverter.convertAppValueToDbValue as jest.Mock + ).mockReturnValue(convertedValue); + + jest.spyOn(keyValuePairRepository, 'insert').mockRejectedValue(error); + + await expect(service.set(key, value)).rejects.toThrow( + ConfigVariableException, + ); + await expect(service.set(key, value)).rejects.toThrow( + `Failed to save config variable ${key}: Insert error`, + ); + await expect(service.set(key, value)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.INTERNAL_ERROR, + }); }); it('should encrypt sensitive string values', async () => { @@ -407,6 +408,7 @@ describe('ConfigStorageService', () => { const encryptedValue = 'encrypted:sensitive-value'; jest.spyOn(keyValuePairRepository, 'findOne').mockResolvedValue(null); + jest.spyOn(TypedReflect, 'getMetadata').mockReturnValue({ [key]: { isSensitive: true, @@ -436,12 +438,13 @@ describe('ConfigStorageService', () => { ); }); - it('should handle encryption errors gracefully', async () => { + it('should handle encryption failure in set() by using unconverted value', async () => { const key = 'SENSITIVE_CONFIG' as keyof ConfigVariables; const value = 'sensitive-value'; - const convertedValue = 'sensitive-value'; + const convertedValue = 'converted-value'; jest.spyOn(keyValuePairRepository, 'findOne').mockResolvedValue(null); + jest.spyOn(TypedReflect, 'getMetadata').mockReturnValue({ [key]: { isSensitive: true, @@ -492,7 +495,15 @@ describe('ConfigStorageService', () => { jest.spyOn(keyValuePairRepository, 'delete').mockRejectedValue(error); - await expect(service.delete(key)).rejects.toThrow('Delete error'); + await expect(service.delete(key)).rejects.toThrow( + ConfigVariableException, + ); + await expect(service.delete(key)).rejects.toThrow( + `Failed to delete config variable ${key}: Delete error`, + ); + await expect(service.delete(key)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.INTERNAL_ERROR, + }); }); }); @@ -552,7 +563,13 @@ describe('ConfigStorageService', () => { jest.spyOn(keyValuePairRepository, 'find').mockRejectedValue(error); - await expect(service.loadAll()).rejects.toThrow('Find error'); + await expect(service.loadAll()).rejects.toThrow(ConfigVariableException); + await expect(service.loadAll()).rejects.toThrow( + 'Failed to load all config variables: Find error', + ); + await expect(service.loadAll()).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.INTERNAL_ERROR, + }); }); describe('Null Value Handling', () => { @@ -649,7 +666,10 @@ describe('ConfigStorageService', () => { throw new Error('Invalid boolean value'); }); - await expect(service.get(key)).rejects.toThrow('Invalid boolean value'); + await expect(service.get(key)).rejects.toThrow(ConfigVariableException); + await expect(service.get(key)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.VALIDATION_FAILED, + }); }); it('should enforce correct types for string config variables', async () => { @@ -668,7 +688,10 @@ describe('ConfigStorageService', () => { throw new Error('Invalid string value'); }); - await expect(service.get(key)).rejects.toThrow('Invalid string value'); + await expect(service.get(key)).rejects.toThrow(ConfigVariableException); + await expect(service.get(key)).rejects.toMatchObject({ + code: ConfigVariableExceptionCode.VALIDATION_FAILED, + }); }); }); @@ -725,78 +748,5 @@ describe('ConfigStorageService', () => { expect(keyValuePairRepository.delete).toHaveBeenCalledTimes(2); }); }); - - describe('Database Connection Issues', () => { - it('should handle database connection failures in get', async () => { - const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; - const error = new Error('Database connection failed'); - - jest.spyOn(keyValuePairRepository, 'findOne').mockRejectedValue(error); - - await expect(service.get(key)).rejects.toThrow( - 'Database connection failed', - ); - }); - - it('should handle database connection failures in set', async () => { - const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; - const value = true; - const error = new Error('Database connection failed'); - - ( - configValueConverter.convertAppValueToDbValue as jest.Mock - ).mockReturnValue('true'); - jest.spyOn(keyValuePairRepository, 'findOne').mockRejectedValue(error); - - await expect(service.set(key, value)).rejects.toThrow( - 'Database connection failed', - ); - }); - - it('should handle database connection failures in delete', async () => { - const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; - const error = new Error('Database connection failed'); - - jest.spyOn(keyValuePairRepository, 'delete').mockRejectedValue(error); - - await expect(service.delete(key)).rejects.toThrow( - 'Database connection failed', - ); - }); - - it('should handle database connection failures in loadAll', async () => { - const error = new Error('Database connection failed'); - - jest.spyOn(keyValuePairRepository, 'find').mockRejectedValue(error); - - await expect(service.loadAll()).rejects.toThrow( - 'Database connection failed', - ); - }); - - it('should handle database operation timeouts', async () => { - const key = 'AUTH_PASSWORD_ENABLED' as keyof ConfigVariables; - const error = new Error('Database operation timed out'); - - let rejectPromise: ((error: Error) => void) | undefined; - const timeoutPromise = new Promise((_, reject) => { - rejectPromise = reject; - }); - - jest - .spyOn(keyValuePairRepository, 'findOne') - .mockReturnValue(timeoutPromise); - - const promise = service.get(key); - - // Simulate timeout by rejecting the promise - if (!rejectPromise) { - throw new Error('Reject function not assigned'); - } - rejectPromise(error); - - await expect(promise).rejects.toThrow('Database operation timed out'); - }); - }); }); }); diff --git a/packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts b/packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts index caff926d6..45eebfbf8 100644 --- a/packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts +++ b/packages/twenty-server/src/engine/core-modules/twenty-config/storage/config-storage.service.ts @@ -15,6 +15,10 @@ import { ConfigVariables } from 'src/engine/core-modules/twenty-config/config-va import { ConfigValueConverterService } from 'src/engine/core-modules/twenty-config/conversion/config-value-converter.service'; import { EnvironmentConfigDriver } from 'src/engine/core-modules/twenty-config/drivers/environment-config.driver'; import { ConfigVariableType } from 'src/engine/core-modules/twenty-config/enums/config-variable-type.enum'; +import { + ConfigVariableException, + ConfigVariableExceptionCode, +} from 'src/engine/core-modules/twenty-config/twenty-config.exception'; import { TypedReflect } from 'src/utils/typed-reflect'; import { ConfigStorageInterface } from './interfaces/config-storage.interface'; @@ -51,12 +55,6 @@ export class ConfigStorageService implements ConfigStorageInterface { ]; } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - private logAndRethrow(message: string, error: any): never { - this.logger.error(message, error); - throw error; - } - private async convertAndSecureValue( // eslint-disable-next-line @typescript-eslint/no-explicit-any value: any, @@ -86,21 +84,18 @@ export class ConfigStorageService implements ConfigStorageInterface { ? decryptText(convertedValue, appSecret) : encryptText(convertedValue, appSecret); } catch (error) { - this.logger.error( - `Failed to ${isDecrypt ? 'decrypt' : 'encrypt'} value for key ${ + this.logger.debug( + `${isDecrypt ? 'Decryption' : 'Encryption'} failed for key ${ key as string - }`, - error, + }: ${error.message}. Using original value.`, ); return convertedValue; } } catch (error) { - this.logAndRethrow( - `Failed to convert value ${ - isDecrypt ? 'from DB' : 'to DB' - } for key ${key as string}`, - error, + throw new ConfigVariableException( + `Failed to convert value for key ${key as string}: ${error.message}`, + ConfigVariableExceptionCode.VALIDATION_FAILED, ); } } @@ -123,7 +118,14 @@ export class ConfigStorageService implements ConfigStorageInterface { return await this.convertAndSecureValue(result.value, key, true); } catch (error) { - this.logAndRethrow(`Failed to get config for ${key as string}`, error); + if (error instanceof ConfigVariableException) { + throw error; + } + + throw new ConfigVariableException( + `Failed to retrieve config variable ${key as string}: ${error instanceof Error ? error.message : String(error)}`, + ConfigVariableExceptionCode.INTERNAL_ERROR, + ); } } @@ -153,7 +155,14 @@ export class ConfigStorageService implements ConfigStorageInterface { }); } } catch (error) { - this.logAndRethrow(`Failed to set config for ${key as string}`, error); + if (error instanceof ConfigVariableException) { + throw error; + } + + throw new ConfigVariableException( + `Failed to save config variable ${key as string}: ${error instanceof Error ? error.message : String(error)}`, + ConfigVariableExceptionCode.INTERNAL_ERROR, + ); } } @@ -163,7 +172,10 @@ export class ConfigStorageService implements ConfigStorageInterface { this.getConfigVariableWhereClause(key as string), ); } catch (error) { - this.logAndRethrow(`Failed to delete config for ${key as string}`, error); + throw new ConfigVariableException( + `Failed to delete config variable ${key as string}: ${error instanceof Error ? error.message : String(error)}`, + ConfigVariableExceptionCode.INTERNAL_ERROR, + ); } } @@ -195,10 +207,10 @@ export class ConfigStorageService implements ConfigStorageInterface { result.set(key, value); } } catch (error) { - this.logger.error( - `Failed to process config value for key ${key as string}`, - error, + this.logger.debug( + `Skipping invalid config value for key ${key as string}: ${error instanceof Error ? error.message : String(error)}`, ); + continue; } } @@ -206,7 +218,10 @@ export class ConfigStorageService implements ConfigStorageInterface { return result; } catch (error) { - this.logAndRethrow('Failed to load all config variables', error); + throw new ConfigVariableException( + `Failed to load all config variables: ${error instanceof Error ? error.message : String(error)}`, + ConfigVariableExceptionCode.INTERNAL_ERROR, + ); } } } diff --git a/packages/twenty-server/test/integration/twenty-config/constants/config-test-keys.constants.ts b/packages/twenty-server/test/integration/twenty-config/constants/config-test-keys.constants.ts index e727167ae..8e6e430ea 100644 --- a/packages/twenty-server/test/integration/twenty-config/constants/config-test-keys.constants.ts +++ b/packages/twenty-server/test/integration/twenty-config/constants/config-test-keys.constants.ts @@ -13,4 +13,4 @@ export const TEST_KEY_METRICS: ConfigKey = 'HEALTH_METRICS_TIME_WINDOW_IN_MINUTES'; export const TEST_KEY_ENV_ONLY: ConfigKey = 'PG_DATABASE_URL'; export const TEST_KEY_NONEXISTENT = 'NONEXISTENT_CONFIG_KEY'; -export const TEST_KEY_STRING_VALUE = 'EMAIL_FROM_NAME'; +export const TEST_KEY_STRING_VALUE: ConfigKey = 'EMAIL_FROM_NAME'; diff --git a/packages/twenty-server/test/integration/twenty-config/twenty-config.integration-spec.ts b/packages/twenty-server/test/integration/twenty-config/twenty-config.integration-spec.ts index f9b60f523..b9c525a4e 100644 --- a/packages/twenty-server/test/integration/twenty-config/twenty-config.integration-spec.ts +++ b/packages/twenty-server/test/integration/twenty-config/twenty-config.integration-spec.ts @@ -257,24 +257,24 @@ describe('TwentyConfig Integration', () => { it('should reject updating config variables with invalid types', async () => { await createConfigVariable({ input: { - key: 'NODE_PORT', - value: 3000, + key: TEST_KEY_DEFAULT, + value: true, }, }); const updateResult = await updateConfigVariable({ input: { - key: 'NODE_PORT', - value: 'not-a-number', + key: TEST_KEY_DEFAULT, + value: 'not-a-boolean', }, expectToFail: true, }); expect(updateResult.errors).toBeDefined(); - expect(updateResult.errors[0].message).toContain('Expected number'); + expect(updateResult.errors[0].message).toContain('Expected boolean'); await deleteConfigVariable({ - input: { key: 'NODE_PORT' }, + input: { key: TEST_KEY_DEFAULT }, }); }); }); diff --git a/packages/twenty-utils/dangerfile.ts b/packages/twenty-utils/dangerfile.ts index df7a78531..187bf8b36 100644 --- a/packages/twenty-utils/dangerfile.ts +++ b/packages/twenty-utils/dangerfile.ts @@ -18,16 +18,15 @@ if (packageChanged && !lockfileChanged) { warn(`${message} - ${idea}`); } -// Check if .env.example was changed, but not config variable documentation -const envChanged = - danger.git.modified_files.find((x) => x.includes('.env.example')) || - danger.git.modified_files.find((x) => x.includes('twenty-config.service.ts')); -const envDocsChanged = danger.git.modified_files.includes('self-hosting.mdx'); -if (envChanged && !envDocsChanged) { - const message = - 'Changes were made to the config variables, but not to the documentation'; - const idea = - 'Please review your changes and check if a change needs to be documented!'; +// Check environment configuration changes +const envExampleChanged = danger.git.modified_files.find((x) => + x.includes('.env.example') +); + +// Check if .env.example was changed +if (envExampleChanged) { + const message = 'Changes were made to .env.example'; + const idea = 'Please make sure any new environment variables are properly documented with metadata in config-variables.ts'; warn(`${message} - ${idea}`); }