From f7081a6a1063ced64765ad53dd03c9da63971181 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Tue, 1 Oct 2024 14:27:39 +0200 Subject: [PATCH 01/10] BC-8092 - implement config validation --- package-lock.json | 35 +++++++++++ package.json | 2 + src/infra/auth-guard/auth-guard.module.ts | 4 +- .../auth-guard/config/x-api-key.config.ts | 9 ++- .../strategy/x-api-key.strategy.spec.ts | 19 +++--- .../auth-guard/strategy/x-api-key.strategy.ts | 5 +- .../authorization/authorization.config.ts | 6 ++ .../authorization/authorization.module.ts | 4 +- .../authorization.service.spec.ts | 11 ++-- .../authorization/authorization.service.ts | 8 +-- .../configuration/configuration.module.ts | 22 +++++++ .../configuration.service.spec.ts | 59 +++++++++++++++++++ .../configuration/configuration.service.ts | 39 ++++++++++++ src/infra/configuration/index.ts | 1 + src/infra/redis/redis.config.ts | 21 +++++++ src/infra/redis/redis.module.ts | 4 +- src/infra/redis/redis.service.spec.ts | 58 ++++++++---------- src/infra/redis/redis.service.ts | 23 ++++---- src/infra/storage/storage.config.ts | 9 +++ src/infra/storage/storage.module.ts | 4 +- src/infra/storage/storage.service.ts | 8 +-- .../api/test/tldraw-document.api.spec.ts | 10 ++-- src/modules/server/api/websocket.gateway.ts | 11 ++-- src/modules/server/server.config.ts | 12 ++++ src/modules/server/server.module.ts | 9 +-- .../server/service/tldraw-document.service.ts | 2 +- src/modules/worker/worker.config.ts | 3 + src/modules/worker/worker.module.ts | 5 +- 28 files changed, 306 insertions(+), 97 deletions(-) create mode 100644 src/infra/authorization/authorization.config.ts create mode 100644 src/infra/configuration/configuration.module.ts create mode 100644 src/infra/configuration/configuration.service.spec.ts create mode 100644 src/infra/configuration/configuration.service.ts create mode 100644 src/infra/configuration/index.ts create mode 100644 src/infra/redis/redis.config.ts create mode 100644 src/infra/storage/storage.config.ts create mode 100644 src/modules/server/server.config.ts create mode 100644 src/modules/worker/worker.config.ts diff --git a/package-lock.json b/package-lock.json index c88406ab..7622008f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,6 +16,8 @@ "@nestjs/passport": "^10.0.3", "@nestjs/platform-express": "^10.4.1", "@y/redis": "github:hpi-schul-cloud/y-redis#7d48e08d18ec78c9ab90063a7d867ec7f191319c", + "class-transformer": "^0.5.1", + "class-validator": "^0.14.1", "ioredis": "^5.4.1", "passport": "^0.7.0", "passport-headerapikey": "^1.2.2", @@ -2232,6 +2234,11 @@ "@types/superagent": "^8.1.0" } }, + "node_modules/@types/validator": { + "version": "13.12.2", + "resolved": "https://registry.npmjs.org/@types/validator/-/validator-13.12.2.tgz", + "integrity": "sha512-6SlHBzUW8Jhf3liqrGGXyTJSIFe4nqlJ5A5KaMZ2l/vbM3Wh3KSybots/wfWVzNLK4D1NZluDlSQIbIEPx6oyA==" + }, "node_modules/@types/yargs": { "version": "17.0.33", "resolved": "https://registry.npmjs.org/@types/yargs/-/yargs-17.0.33.tgz", @@ -3488,6 +3495,21 @@ "integrity": "sha512-a3KdPAANPbNE4ZUv9h6LckSl9zLsYOP4MBmhIPkRaeyybt+r4UghLvq+xw/YwUcC1gqylCkL4rdVs3Lwupjm4Q==", "dev": true }, + "node_modules/class-transformer": { + "version": "0.5.1", + "resolved": "https://registry.npmjs.org/class-transformer/-/class-transformer-0.5.1.tgz", + "integrity": "sha512-SQa1Ws6hUbfC98vKGxZH3KFY0Y1lm5Zm0SY8XX9zbK7FJCyVEac3ATW0RIpwzW+oOfmHE5PMPufDG9hCfoEOMw==" + }, + "node_modules/class-validator": { + "version": "0.14.1", + "resolved": "https://registry.npmjs.org/class-validator/-/class-validator-0.14.1.tgz", + "integrity": "sha512-2VEG9JICxIqTpoK1eMzZqaV+u/EiwEJkMGzTrZf6sU/fwsnOITVgYJ8yojSy6CaXtO9V0Cc6ZQZ8h8m4UBuLwQ==", + "dependencies": { + "@types/validator": "^13.11.8", + "libphonenumber-js": "^1.10.53", + "validator": "^13.9.0" + } + }, "node_modules/cli-cursor": { "version": "3.1.0", "resolved": "https://registry.npmjs.org/cli-cursor/-/cli-cursor-3.1.0.tgz", @@ -6557,6 +6579,11 @@ "url": "https://github.com/sponsors/dmonad" } }, + "node_modules/libphonenumber-js": { + "version": "1.11.9", + "resolved": "https://registry.npmjs.org/libphonenumber-js/-/libphonenumber-js-1.11.9.tgz", + "integrity": "sha512-Zs5wf5HaWzW2/inlupe2tstl0I/Tbqo7lH20ZLr6Is58u7Dz2n+gRFGNlj9/gWxFvNfp9+YyDsiegjNhdixB9A==" + }, "node_modules/lines-and-columns": { "version": "1.2.4", "resolved": "https://registry.npmjs.org/lines-and-columns/-/lines-and-columns-1.2.4.tgz", @@ -9078,6 +9105,14 @@ "node": ">=10.12.0" } }, + "node_modules/validator": { + "version": "13.12.0", + "resolved": "https://registry.npmjs.org/validator/-/validator-13.12.0.tgz", + "integrity": "sha512-c1Q0mCiPlgdTVVVIJIrBuxNicYE+t/7oKeI9MWLj3fh/uq2Pxh/3eeWbVZ4OcGW1TUf53At0njHw5SMdA3tmMg==", + "engines": { + "node": ">= 0.10" + } + }, "node_modules/vary": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", diff --git a/package.json b/package.json index 28e525cc..fd2dc522 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,8 @@ "@nestjs/passport": "^10.0.3", "@nestjs/platform-express": "^10.4.1", "@y/redis": "github:hpi-schul-cloud/y-redis#7d48e08d18ec78c9ab90063a7d867ec7f191319c", + "class-transformer": "^0.5.1", + "class-validator": "^0.14.1", "ioredis": "^5.4.1", "passport": "^0.7.0", "passport-headerapikey": "^1.2.2", diff --git a/src/infra/auth-guard/auth-guard.module.ts b/src/infra/auth-guard/auth-guard.module.ts index c175934c..b0e674e2 100644 --- a/src/infra/auth-guard/auth-guard.module.ts +++ b/src/infra/auth-guard/auth-guard.module.ts @@ -1,9 +1,11 @@ import { Module } from '@nestjs/common'; import { PassportModule } from '@nestjs/passport'; +import { ConfigurationModule } from '../configuration/configuration.module.js'; +import { XApiKeyConfig } from './config/x-api-key.config.js'; import { XApiKeyStrategy } from './strategy/index.js'; @Module({ - imports: [PassportModule], + imports: [PassportModule, ConfigurationModule.register(XApiKeyConfig)], providers: [XApiKeyStrategy], exports: [], }) diff --git a/src/infra/auth-guard/config/x-api-key.config.ts b/src/infra/auth-guard/config/x-api-key.config.ts index 97c189e2..c8628096 100644 --- a/src/infra/auth-guard/config/x-api-key.config.ts +++ b/src/infra/auth-guard/config/x-api-key.config.ts @@ -1,3 +1,8 @@ -export interface XApiKeyConfig { - ADMIN_API__ALLOWED_API_KEYS: string[]; +import { Transform } from 'class-transformer'; +import { IsArray } from 'class-validator'; + +export class XApiKeyConfig { + @Transform(({ value }) => value.split(',').map((part: string) => (part.split(':').pop() ?? '').trim())) + @IsArray() + public ADMIN_API__ALLOWED_API_KEYS!: string[]; } diff --git a/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts b/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts index b6664151..4a3e9af1 100644 --- a/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts +++ b/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts @@ -1,6 +1,5 @@ import { createMock } from '@golevelup/ts-jest'; import { UnauthorizedException } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; import { Test, TestingModule } from '@nestjs/testing'; import { XApiKeyConfig } from '../config/x-api-key.config.js'; import { XApiKeyStrategy } from './x-api-key.strategy.js'; @@ -8,7 +7,7 @@ import { XApiKeyStrategy } from './x-api-key.strategy.js'; describe('XApiKeyStrategy', () => { let module: TestingModule; let strategy: XApiKeyStrategy; - let configService: ConfigService; + let config: XApiKeyConfig; beforeAll(async () => { module = await Test.createTestingModule({ @@ -16,16 +15,14 @@ describe('XApiKeyStrategy', () => { providers: [ XApiKeyStrategy, { - provide: ConfigService, - useValue: createMock>({ - get: () => ['7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4'], - }), + provide: XApiKeyConfig, + useValue: createMock(), }, ], }).compile(); strategy = module.get(XApiKeyStrategy); - configService = module.get(ConfigService); + config = module.get(XApiKeyConfig); }); afterAll(async () => { @@ -41,33 +38,35 @@ describe('XApiKeyStrategy', () => { describe('when a valid api key is provided', () => { const setup = () => { const CORRECT_API_KEY = '7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4'; + config.ADMIN_API__ALLOWED_API_KEYS = [CORRECT_API_KEY]; return { CORRECT_API_KEY, done }; }; it('should do nothing', () => { const { CORRECT_API_KEY } = setup(); strategy.validate(CORRECT_API_KEY, done); - expect(done).toBeCalledWith(null, true); + expect(done).toHaveBeenCalledWith(null, true); }); }); describe('when a invalid api key is provided', () => { const setup = () => { const INVALID_API_KEY = '7ccd4e11-c6f6-48b0-81eb-cccf7922e7a4BAD'; + config.ADMIN_API__ALLOWED_API_KEYS = [INVALID_API_KEY]; return { INVALID_API_KEY, done }; }; it('should throw error', () => { const { INVALID_API_KEY } = setup(); strategy.validate(INVALID_API_KEY, done); - expect(done).toBeCalledWith(new UnauthorizedException(), null); + expect(done).toHaveBeenCalledWith(new UnauthorizedException(), null); }); }); }); describe('constructor', () => { it('should create strategy', () => { - const ApiKeyStrategy = new XApiKeyStrategy(configService); + const ApiKeyStrategy = new XApiKeyStrategy(config); expect(ApiKeyStrategy).toBeDefined(); expect(ApiKeyStrategy).toBeInstanceOf(XApiKeyStrategy); }); diff --git a/src/infra/auth-guard/strategy/x-api-key.strategy.ts b/src/infra/auth-guard/strategy/x-api-key.strategy.ts index 7c6e2e10..69640582 100644 --- a/src/infra/auth-guard/strategy/x-api-key.strategy.ts +++ b/src/infra/auth-guard/strategy/x-api-key.strategy.ts @@ -1,5 +1,4 @@ import { Injectable, UnauthorizedException } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; import { PassportStrategy } from '@nestjs/passport'; import { Strategy } from 'passport-headerapikey/lib/Strategy.js'; import { XApiKeyConfig } from '../config/index.js'; @@ -9,9 +8,9 @@ import { StrategyType } from '../interface/index.js'; export class XApiKeyStrategy extends PassportStrategy(Strategy, StrategyType.API_KEY) { private readonly allowedApiKeys: string[]; - public constructor(private readonly configService: ConfigService) { + public constructor(private readonly config: XApiKeyConfig) { super({ header: 'X-API-KEY' }, false); - this.allowedApiKeys = this.configService.get('ADMIN_API__ALLOWED_API_KEYS'); + this.allowedApiKeys = this.config.ADMIN_API__ALLOWED_API_KEYS; } public validate(apiKey: string, done: (error: Error | null, data: boolean | null) => void): void { diff --git a/src/infra/authorization/authorization.config.ts b/src/infra/authorization/authorization.config.ts new file mode 100644 index 00000000..0e8cf560 --- /dev/null +++ b/src/infra/authorization/authorization.config.ts @@ -0,0 +1,6 @@ +import { IsUrl } from 'class-validator'; + +export class AuthorizationConfig { + @IsUrl({ require_tld: false }) + public API_HOST!: string; +} diff --git a/src/infra/authorization/authorization.module.ts b/src/infra/authorization/authorization.module.ts index b30dcdd1..7277fe82 100644 --- a/src/infra/authorization/authorization.module.ts +++ b/src/infra/authorization/authorization.module.ts @@ -1,9 +1,11 @@ import { Module } from '@nestjs/common'; +import { ConfigurationModule } from '../configuration/configuration.module.js'; import { LoggerModule } from '../logging/logger.module.js'; +import { AuthorizationConfig } from './authorization.config.js'; import { AuthorizationService } from './authorization.service.js'; @Module({ - imports: [LoggerModule], + imports: [LoggerModule, ConfigurationModule.register(AuthorizationConfig)], providers: [AuthorizationService], exports: [AuthorizationService], }) diff --git a/src/infra/authorization/authorization.service.spec.ts b/src/infra/authorization/authorization.service.spec.ts index bac09569..6cc47f26 100644 --- a/src/infra/authorization/authorization.service.spec.ts +++ b/src/infra/authorization/authorization.service.spec.ts @@ -1,22 +1,23 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { ConfigService } from '@nestjs/config'; import { Test, TestingModule } from '@nestjs/testing'; import { HttpRequest } from 'uws'; import { Logger } from '../logging/logger.js'; +import { AuthorizationConfig } from './authorization.config.js'; import { AuthorizationService } from './authorization.service.js'; describe(AuthorizationService.name, () => { let module: TestingModule; let service: AuthorizationService; - let configService: DeepMocked; beforeAll(async () => { module = await Test.createTestingModule({ providers: [ AuthorizationService, { - provide: ConfigService, - useValue: createMock(), + provide: AuthorizationConfig, + useValue: createMock({ + API_HOST: 'http://localhost:3000', + }), }, { provide: Logger, @@ -26,7 +27,6 @@ describe(AuthorizationService.name, () => { }).compile(); service = module.get(AuthorizationService); - configService = module.get(ConfigService); }); afterAll(async () => { @@ -41,7 +41,6 @@ describe(AuthorizationService.name, () => { const req: DeepMocked = createMock(); jest.spyOn(req, 'getParameter').mockReturnValue(roomId); jest.spyOn(req, 'getHeader').mockReturnValue(cookies); - configService.getOrThrow.mockReturnValue('API_HOST'); const fetchSpy = jest.spyOn(global, 'fetch'); return { req, fetchSpy }; diff --git a/src/infra/authorization/authorization.service.ts b/src/infra/authorization/authorization.service.ts index 9f50df91..5d5ae2ee 100644 --- a/src/infra/authorization/authorization.service.ts +++ b/src/infra/authorization/authorization.service.ts @@ -1,15 +1,15 @@ import { Injectable } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; import { HttpRequest } from 'uws'; import { Logger } from '../logging/logger.js'; +import { AuthorizationConfig } from './authorization.config.js'; import { ResponsePayload } from './interfaces/response.payload.js'; import { ResponsePayloadBuilder } from './response.builder.js'; @Injectable() export class AuthorizationService { public constructor( - private configService: ConfigService, - private logger: Logger, + private readonly config: AuthorizationConfig, + private readonly logger: Logger, ) { logger.setContext(AuthorizationService.name); } @@ -49,7 +49,7 @@ export class AuthorizationService { } private async fetchAuthorization(room: string, token: string): Promise { - const apiHost = this.configService.getOrThrow('API_HOST'); + const apiHost = this.config.API_HOST; const requestOptions = this.createAuthzRequestOptions(room, token); const response = await fetch(`${apiHost}/api/v3/authorization/by-reference`, requestOptions); diff --git a/src/infra/configuration/configuration.module.ts b/src/infra/configuration/configuration.module.ts new file mode 100644 index 00000000..0a330bc3 --- /dev/null +++ b/src/infra/configuration/configuration.module.ts @@ -0,0 +1,22 @@ +import { DynamicModule, Module } from '@nestjs/common'; +import { ConfigModule } from '@nestjs/config'; +import { Configuration } from './configuration.service.js'; + +@Module({}) +export class ConfigurationModule { + public static register(Constructor: new () => T): DynamicModule { + return { + imports: [ConfigModule.forRoot({ isGlobal: true, cache: true })], + providers: [ + Configuration, + { + provide: Constructor, + useFactory: (config: Configuration): T => config.getAllValidConfigsByType(Constructor), + inject: [Configuration], + }, + ], + exports: [Configuration, Constructor], + module: ConfigurationModule, + }; + } +} diff --git a/src/infra/configuration/configuration.service.spec.ts b/src/infra/configuration/configuration.service.spec.ts new file mode 100644 index 00000000..8511494a --- /dev/null +++ b/src/infra/configuration/configuration.service.spec.ts @@ -0,0 +1,59 @@ +import { createMock } from '@golevelup/ts-jest'; +import { ConfigService } from '@nestjs/config'; +import { Test, TestingModule } from '@nestjs/testing'; +import { IsString } from 'class-validator'; +import { Configuration } from './configuration.service.js'; + +class TestConfig { + @IsString() + public TEST_VALUE!: string; +} + +describe(Configuration.name, () => { + let module: TestingModule; + let service: Configuration; + let configService: ConfigService; + + beforeAll(async () => { + module = await Test.createTestingModule({ + providers: [ + Configuration, + { + provide: ConfigService, + useValue: createMock(), + }, + ], + }).compile(); + + service = module.get(Configuration); + configService = module.get(ConfigService); + }); + + afterAll(async () => { + await module.close(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('getAllValidConfigsByType', () => { + describe('when value is valid', () => { + it('should return valid configs', () => { + jest.spyOn(configService, 'get').mockReturnValueOnce('test'); + + const result = service.getAllValidConfigsByType(TestConfig); + + expect(result).toEqual({ TEST_VALUE: 'test' }); + }); + }); + + describe('when value is not valid', () => { + it('should throw error', () => { + jest.spyOn(configService, 'get').mockReturnValueOnce(123); + + expect(() => service.getAllValidConfigsByType(TestConfig)).toThrow(/isString/); + }); + }); + }); +}); diff --git a/src/infra/configuration/configuration.service.ts b/src/infra/configuration/configuration.service.ts new file mode 100644 index 00000000..3e71fed0 --- /dev/null +++ b/src/infra/configuration/configuration.service.ts @@ -0,0 +1,39 @@ +import { Injectable } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; +import { plainToInstance } from 'class-transformer'; +import { validateSync } from 'class-validator'; + +@Injectable() +export class Configuration { + public constructor(private readonly configService: ConfigService) {} + + public getAllValidConfigsByType(Constructor: new () => T): T { + const config = new Constructor(); + const configKeys = Object.keys(config); + + const configValues = configKeys.reduce((acc: Record, key) => { + const value = this.configService.get(key); + + if (value) { + acc[key] = value; + } + + return acc; + }, {}); + + const validatedConfig = this.validate(configValues, Constructor); + + return validatedConfig; + } + + private validate(config: Record, Constructor: new () => T): T { + const validatedConfig = plainToInstance(Constructor, config); + const errors = validateSync(validatedConfig); + + if (errors.length > 0) { + throw new Error(errors.toString()); + } + + return validatedConfig; + } +} diff --git a/src/infra/configuration/index.ts b/src/infra/configuration/index.ts new file mode 100644 index 00000000..923719d3 --- /dev/null +++ b/src/infra/configuration/index.ts @@ -0,0 +1 @@ +export * from './configuration.module.js'; diff --git a/src/infra/redis/redis.config.ts b/src/infra/redis/redis.config.ts new file mode 100644 index 00000000..47692c55 --- /dev/null +++ b/src/infra/redis/redis.config.ts @@ -0,0 +1,21 @@ +import { IsOptional, IsString, IsUrl } from 'class-validator'; + +export class RedisConfig { + @IsUrl({ protocols: ['redis'], require_tld: false }) + @IsOptional() + public REDIS!: string; + + @IsString() + @IsOptional() + public REDIS_SENTINEL_SERVICE_NAME!: string; + + @IsString() + public REDIS_PREFIX = 'y'; + + @IsString() + public REDIS_SENTINEL_NAME = 'mymaster'; + + @IsString() + @IsOptional() + public REDIS_SENTINEL_PASSWORD!: string; +} diff --git a/src/infra/redis/redis.module.ts b/src/infra/redis/redis.module.ts index b73feac9..c4fa6c88 100644 --- a/src/infra/redis/redis.module.ts +++ b/src/infra/redis/redis.module.ts @@ -1,9 +1,11 @@ import { Module } from '@nestjs/common'; +import { ConfigurationModule } from '../configuration/configuration.module.js'; import { LoggerModule } from '../logging/logger.module.js'; +import { RedisConfig } from './redis.config.js'; import { RedisService } from './redis.service.js'; @Module({ - imports: [LoggerModule], + imports: [LoggerModule, ConfigurationModule.register(RedisConfig)], providers: [RedisService], exports: [RedisService], }) diff --git a/src/infra/redis/redis.service.spec.ts b/src/infra/redis/redis.service.spec.ts index e45e9a58..d5debae0 100644 --- a/src/infra/redis/redis.service.spec.ts +++ b/src/infra/redis/redis.service.spec.ts @@ -1,33 +1,11 @@ -import { createMock, DeepMocked } from '@golevelup/ts-jest'; -import { ConfigService } from '@nestjs/config'; -import { Test } from '@nestjs/testing'; +import { createMock } from '@golevelup/ts-jest'; +import { Redis } from 'ioredis'; import * as util from 'util'; import { Logger } from '../logging/logger.js'; +import { RedisConfig } from './redis.config.js'; import { RedisService } from './redis.service.js'; describe('Redis Service', () => { - let service: RedisService; - let configService: DeepMocked; - - beforeAll(async () => { - const moduleFixture = await Test.createTestingModule({ - providers: [ - RedisService, - { - provide: ConfigService, - useValue: createMock(), - }, - { - provide: Logger, - useValue: createMock(), - }, - ], - }).compile(); - - service = moduleFixture.get(RedisService); - configService = moduleFixture.get(ConfigService); - }); - beforeEach(() => { jest.resetAllMocks(); }); @@ -39,13 +17,15 @@ describe('Redis Service', () => { const sentinelServiceName = 'serviceName'; const sentinelName = 'sentinelName'; const sentinelPassword = 'sentinelPassword'; - const redisPrefix = 'y'; + const redisPrefix = 'A'; + + const config = new RedisConfig(); - configService.get.mockReturnValueOnce(sentinelServiceName); - configService.get.mockReturnValueOnce(redisPrefix); - configService.get.mockReturnValueOnce(sentinelName); - configService.get.mockReturnValueOnce(sentinelPassword); - configService.get.mockReturnValueOnce(sentinelPassword); + config.REDIS = sentinelServiceName; + config.REDIS_SENTINEL_SERVICE_NAME = sentinelServiceName; + config.REDIS_PREFIX = redisPrefix; + config.REDIS_SENTINEL_NAME = sentinelName; + config.REDIS_SENTINEL_PASSWORD = sentinelPassword; const name1 = 'name1'; const name2 = 'name2'; @@ -60,17 +40,27 @@ describe('Redis Service', () => { //const redisMock = createMock(); //jest.spyOn(ioredisModule, 'Redis').mockReturnValueOnce(redisMock); + const logger = createMock(); + const service = new RedisService(config, logger); - return { resolveSrv, sentinelServiceName }; + return { resolveSrv, sentinelServiceName, service }; }; - it.only('calls resolveSrv', async () => { - const { resolveSrv, sentinelServiceName } = setup(); + it('calls resolveSrv', async () => { + const { resolveSrv, sentinelServiceName, service } = setup(); await service.createRedisInstance(); expect(resolveSrv).toHaveBeenLastCalledWith(sentinelServiceName); }); + + it('creates a new Redis instance', async () => { + const { service } = setup(); + + const redisInstance = await service.createRedisInstance(); + + expect(redisInstance).toBeInstanceOf(Redis); + }); }); }); }); diff --git a/src/infra/redis/redis.service.ts b/src/infra/redis/redis.service.ts index 9524493c..37ddff84 100644 --- a/src/infra/redis/redis.service.ts +++ b/src/infra/redis/redis.service.ts @@ -1,27 +1,26 @@ import { Injectable } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; import * as dns from 'dns'; import { Redis } from 'ioredis'; import * as util from 'util'; import { Logger } from '../logging/logger.js'; +import { RedisConfig } from './redis.config.js'; @Injectable() export class RedisService { - private sentinelServiceName: string; + private readonly sentinelServiceName: string; + private readonly redisDeletionKey: string; + private readonly redisDeletionActionKey: string; private internalRedisInstance?: Redis; - private redisDeletionKey: string; - private redisDeletionActionKey: string; public constructor( - private configService: ConfigService, - private logger: Logger, + private readonly config: RedisConfig, + private readonly logger: Logger, ) { - this.sentinelServiceName = this.configService.get('REDIS_SENTINEL_SERVICE_NAME') ?? ''; - const redisPrefix = this.configService.get('REDIS_PREFIX') ?? 'y'; + const redisPrefix = this.config.REDIS_PREFIX; + this.sentinelServiceName = this.config.REDIS_SENTINEL_SERVICE_NAME; this.redisDeletionKey = `${redisPrefix}:delete`; this.redisDeletionActionKey = `${redisPrefix}:delete:action`; - this.logger.setContext(RedisService.name); } @@ -60,15 +59,15 @@ export class RedisService { } private createNewRedisInstance(): Redis { - const redisUrl = this.configService.getOrThrow('REDIS'); + const redisUrl = this.config.REDIS; const redisInstance = new Redis(redisUrl); return redisInstance; } private async createRedisSentinelInstance(): Promise { - const sentinelName = this.configService.get('REDIS_SENTINEL_NAME') ?? 'mymaster'; - const sentinelPassword = this.configService.getOrThrow('REDIS_SENTINEL_PASSWORD'); + const sentinelName = this.config.REDIS_SENTINEL_NAME; + const sentinelPassword = this.config.REDIS_SENTINEL_PASSWORD; const sentinels = await this.discoverSentinelHosts(); this.logger.log('Discovered sentinels:', sentinels); diff --git a/src/infra/storage/storage.config.ts b/src/infra/storage/storage.config.ts new file mode 100644 index 00000000..8fe37553 --- /dev/null +++ b/src/infra/storage/storage.config.ts @@ -0,0 +1,9 @@ +import { IsString } from 'class-validator'; + +export class StorageConfig { + @IsString() + public S3_ENDPOINT!: string; + + @IsString() + public S3_BUCKET = 'ydocs'; +} diff --git a/src/infra/storage/storage.module.ts b/src/infra/storage/storage.module.ts index 9a4280ca..7ead28b4 100644 --- a/src/infra/storage/storage.module.ts +++ b/src/infra/storage/storage.module.ts @@ -1,9 +1,11 @@ import { Module } from '@nestjs/common'; +import { ConfigurationModule } from '../configuration/configuration.module.js'; import { LoggerModule } from '../logging/logger.module.js'; +import { StorageConfig } from './storage.config.js'; import { StorageService } from './storage.service.js'; @Module({ - imports: [LoggerModule], + imports: [LoggerModule, ConfigurationModule.register(StorageConfig)], providers: [StorageService], exports: [StorageService], }) diff --git a/src/infra/storage/storage.service.ts b/src/infra/storage/storage.service.ts index ad9e6396..ca943534 100644 --- a/src/infra/storage/storage.service.ts +++ b/src/infra/storage/storage.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; import { Logger } from '../logging/logger.js'; +import { StorageConfig } from './storage.config.js'; @Injectable() export class StorageService { @@ -8,12 +8,12 @@ export class StorageService { private bucketName: string; public constructor( - private configService: ConfigService, + private config: StorageConfig, private logger: Logger, ) { this.logger.setContext(StorageService.name); - this.s3Endpoint = this.configService.getOrThrow('S3_ENDPOINT'); - this.bucketName = this.configService.get('S3_BUCKET') ?? 'ydocs'; + this.s3Endpoint = this.config.S3_ENDPOINT; + this.bucketName = this.config.S3_BUCKET; } public async get(): Promise { diff --git a/src/modules/server/api/test/tldraw-document.api.spec.ts b/src/modules/server/api/test/tldraw-document.api.spec.ts index ffab9ffc..4a5a463c 100644 --- a/src/modules/server/api/test/tldraw-document.api.spec.ts +++ b/src/modules/server/api/test/tldraw-document.api.spec.ts @@ -1,15 +1,15 @@ jest.mock('@y/redis'); +import { createMock } from '@golevelup/ts-jest'; import { INestApplication } from '@nestjs/common'; import { Test } from '@nestjs/testing'; -import { StorageService } from '../../../../infra/storage/storage.service.js'; -import { RedisService } from '../../../../infra/redis/redis.service.js'; import { App } from 'uws'; -import { createMock } from '@golevelup/ts-jest'; +import { RedisService } from '../../../../infra/redis/redis.service.js'; +import { StorageService } from '../../../../infra/storage/storage.service.js'; import { ServerModule } from '../../server.module.js'; import { WebsocketGateway } from '../websocket.gateway.js'; -describe('Tldraw-Document Api Test', () => { +describe.skip('Tldraw-Document Api Test', () => { let app: INestApplication; beforeAll(async () => { @@ -35,7 +35,7 @@ describe('Tldraw-Document Api Test', () => { }); describe('deleteByDocName', () => { - it('true to be true', () => { + it.skip('true to be true', () => { expect(true).toBe(true); }); }); diff --git a/src/modules/server/api/websocket.gateway.ts b/src/modules/server/api/websocket.gateway.ts index 10dfabd4..e7ab9e4a 100644 --- a/src/modules/server/api/websocket.gateway.ts +++ b/src/modules/server/api/websocket.gateway.ts @@ -1,5 +1,4 @@ import { Inject, Injectable, OnModuleDestroy, OnModuleInit } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; // @ts-expect-error - @y/redis is only having jsdoc types import { registerYWebsocketServer } from '@y/redis'; import { TemplatedApp } from 'uws'; @@ -8,6 +7,7 @@ import { Logger } from '../../../infra/logging/logger.js'; import { MetricsService } from '../../../infra/metrics/metrics.service.js'; import { RedisService } from '../../../infra/redis/redis.service.js'; import { StorageService } from '../../../infra/storage/storage.service.js'; +import { ServerConfig } from '../server.config.js'; export const UWS = 'UWS'; @@ -18,7 +18,7 @@ export class WebsocketGateway implements OnModuleInit, OnModuleDestroy { private storage: StorageService, private authorizationService: AuthorizationService, private redisService: RedisService, - private configService: ConfigService, + private config: ServerConfig, private logger: Logger, ) { this.logger.setContext(WebsocketGateway.name); @@ -29,8 +29,8 @@ export class WebsocketGateway implements OnModuleInit, OnModuleDestroy { } public async onModuleInit(): Promise { - const wsPathPrefix = this.configService.get('WS_PATH_PREFIX') ?? ''; - const wsPort = Number(this.configService.get('WS_PORT', { infer: true })) || 3345; + const wsPathPrefix = this.config.WS_PATH_PREFIX; + const wsPort = this.config.WS_PORT; await registerYWebsocketServer( this.webSocketServer, @@ -38,7 +38,7 @@ export class WebsocketGateway implements OnModuleInit, OnModuleDestroy { await this.storage.get(), this.authorizationService.hasPermission.bind(this.authorizationService), { - redisPrefix: this.configService.get('REDIS_PREFIX') ?? 'y', + redisPrefix: this.config.REDIS_PREFIX, openWsCallback: () => this.incOpenConnectionsGauge(), closeWsCallback: () => this.decOpenConnectionsGauge(), }, @@ -52,7 +52,6 @@ export class WebsocketGateway implements OnModuleInit, OnModuleDestroy { }); this.redisService.subscribeToDeleteChannel((message: string) => { - console.log('Received message in delete channel:', message); this.webSocketServer.publish(message, 'action:delete'); }); } diff --git a/src/modules/server/server.config.ts b/src/modules/server/server.config.ts new file mode 100644 index 00000000..6725d41e --- /dev/null +++ b/src/modules/server/server.config.ts @@ -0,0 +1,12 @@ +import { Transform } from 'class-transformer'; +import { IsNumber, IsString } from 'class-validator'; +import { RedisConfig } from '../../infra/redis/redis.config.js'; + +export class ServerConfig extends RedisConfig { + @IsString() + public WS_PATH_PREFIX = ''; + + @IsNumber() + @Transform(({ value }) => parseInt(value)) + public WS_PORT!: number; +} diff --git a/src/modules/server/server.module.ts b/src/modules/server/server.module.ts index 9dac1174..bb66e1ea 100644 --- a/src/modules/server/server.module.ts +++ b/src/modules/server/server.module.ts @@ -1,18 +1,19 @@ import { Module } from '@nestjs/common'; -import { ConfigModule } from '@nestjs/config'; import { App } from 'uws'; +import { AuthGuardModule } from '../../infra/auth-guard/auth-guard.module.js'; import { AuthorizationModule } from '../../infra/authorization/authorization.module.js'; +import { ConfigurationModule } from '../../infra/configuration/configuration.module.js'; import { LoggerModule } from '../../infra/logging/logger.module.js'; import { RedisModule } from '../../infra/redis/index.js'; import { StorageModule } from '../../infra/storage/storage.module.js'; -import { UWS, WebsocketGateway } from './api/websocket.gateway.js'; -import { AuthGuardModule } from '../../infra/auth-guard/auth-guard.module.js'; import { TldrawDocumentController } from './api/tldraw-document.controller.js'; +import { UWS, WebsocketGateway } from './api/websocket.gateway.js'; +import { ServerConfig } from './server.config.js'; import { TldrawDocumentService } from './service/tldraw-document.service.js'; @Module({ imports: [ - ConfigModule.forRoot({ isGlobal: true }), + ConfigurationModule.register(ServerConfig), RedisModule, StorageModule, AuthorizationModule, diff --git a/src/modules/server/service/tldraw-document.service.ts b/src/modules/server/service/tldraw-document.service.ts index ac52c2d4..037ecb1c 100644 --- a/src/modules/server/service/tldraw-document.service.ts +++ b/src/modules/server/service/tldraw-document.service.ts @@ -6,7 +6,7 @@ const UWS = 'UWS'; @Injectable() export class TldrawDocumentService { public constructor( - @Inject(UWS) private webSocketServer: TemplatedApp, + @Inject(UWS) private readonly webSocketServer: TemplatedApp, private readonly redisService: RedisService, ) {} diff --git a/src/modules/worker/worker.config.ts b/src/modules/worker/worker.config.ts new file mode 100644 index 00000000..14fd97c3 --- /dev/null +++ b/src/modules/worker/worker.config.ts @@ -0,0 +1,3 @@ +import { RedisConfig } from '../../infra/redis/redis.config.js'; + +export class WorkerConfig extends RedisConfig {} diff --git a/src/modules/worker/worker.module.ts b/src/modules/worker/worker.module.ts index 66727145..e8e5a097 100644 --- a/src/modules/worker/worker.module.ts +++ b/src/modules/worker/worker.module.ts @@ -1,11 +1,12 @@ import { Module } from '@nestjs/common'; -import { ConfigModule } from '@nestjs/config'; +import { ConfigurationModule } from '../../infra/configuration/configuration.module.js'; import { RedisModule } from '../../infra/redis/redis.module.js'; import { StorageModule } from '../../infra/storage/storage.module.js'; +import { WorkerConfig } from './worker.config.js'; import { WorkerService } from './worker.service.js'; @Module({ - imports: [ConfigModule.forRoot({ isGlobal: true }), RedisModule, StorageModule], + imports: [ConfigurationModule.register(WorkerConfig), RedisModule, StorageModule], providers: [WorkerService], }) export class WorkerModule {} From 5fb4178a61d8904dbe678d5693becf28b356e22c Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Tue, 1 Oct 2024 14:55:56 +0200 Subject: [PATCH 02/10] BC-8092 - fix mocks for Redis and @y/redis --- src/infra/redis/redis.service.spec.ts | 6 ++++++ .../server/api/test/tldraw-document.api.spec.ts | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/infra/redis/redis.service.spec.ts b/src/infra/redis/redis.service.spec.ts index d5debae0..28ff35df 100644 --- a/src/infra/redis/redis.service.spec.ts +++ b/src/infra/redis/redis.service.spec.ts @@ -5,6 +5,12 @@ import { Logger } from '../logging/logger.js'; import { RedisConfig } from './redis.config.js'; import { RedisService } from './redis.service.js'; +jest.mock('ioredis', () => { + return { + Redis: jest.fn(), + }; +}); + describe('Redis Service', () => { beforeEach(() => { jest.resetAllMocks(); diff --git a/src/modules/server/api/test/tldraw-document.api.spec.ts b/src/modules/server/api/test/tldraw-document.api.spec.ts index 4a5a463c..de790bdb 100644 --- a/src/modules/server/api/test/tldraw-document.api.spec.ts +++ b/src/modules/server/api/test/tldraw-document.api.spec.ts @@ -1,4 +1,13 @@ -jest.mock('@y/redis'); +jest.mock('@y/redis', () => { + return { + registerYWebsocketServer: jest.fn(), + Api: jest.fn().mockImplementation(() => { + return { + prototype: jest.fn(), + }; + }), + }; +}); import { createMock } from '@golevelup/ts-jest'; import { INestApplication } from '@nestjs/common'; @@ -9,7 +18,7 @@ import { StorageService } from '../../../../infra/storage/storage.service.js'; import { ServerModule } from '../../server.module.js'; import { WebsocketGateway } from '../websocket.gateway.js'; -describe.skip('Tldraw-Document Api Test', () => { +describe('Tldraw-Document Api Test', () => { let app: INestApplication; beforeAll(async () => { @@ -35,7 +44,7 @@ describe.skip('Tldraw-Document Api Test', () => { }); describe('deleteByDocName', () => { - it.skip('true to be true', () => { + it('true to be true', () => { expect(true).toBe(true); }); }); From 5f355b8f869a49c721269bc6a0364a18e25d7ad5 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Tue, 1 Oct 2024 15:13:50 +0200 Subject: [PATCH 03/10] fix tests --- src/infra/configuration/configuration.module.ts | 2 +- src/infra/storage/storage.config.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/infra/configuration/configuration.module.ts b/src/infra/configuration/configuration.module.ts index 0a330bc3..767df8a4 100644 --- a/src/infra/configuration/configuration.module.ts +++ b/src/infra/configuration/configuration.module.ts @@ -6,7 +6,7 @@ import { Configuration } from './configuration.service.js'; export class ConfigurationModule { public static register(Constructor: new () => T): DynamicModule { return { - imports: [ConfigModule.forRoot({ isGlobal: true, cache: true })], + imports: [ConfigModule.forRoot({ cache: true })], providers: [ Configuration, { diff --git a/src/infra/storage/storage.config.ts b/src/infra/storage/storage.config.ts index 8fe37553..7a674ac3 100644 --- a/src/infra/storage/storage.config.ts +++ b/src/infra/storage/storage.config.ts @@ -1,7 +1,8 @@ -import { IsString } from 'class-validator'; +import { IsOptional, IsString } from 'class-validator'; export class StorageConfig { @IsString() + @IsOptional() public S3_ENDPOINT!: string; @IsString() From 173fbd7bef1a59abaf56a50053fb7865d4c908e2 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Tue, 1 Oct 2024 15:23:19 +0200 Subject: [PATCH 04/10] fixup! fix tests --- src/infra/authorization/authorization.config.ts | 2 +- src/modules/server/server.config.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/infra/authorization/authorization.config.ts b/src/infra/authorization/authorization.config.ts index 0e8cf560..4fe13add 100644 --- a/src/infra/authorization/authorization.config.ts +++ b/src/infra/authorization/authorization.config.ts @@ -2,5 +2,5 @@ import { IsUrl } from 'class-validator'; export class AuthorizationConfig { @IsUrl({ require_tld: false }) - public API_HOST!: string; + public API_HOST = 'http://localhost:3030'; } diff --git a/src/modules/server/server.config.ts b/src/modules/server/server.config.ts index 6725d41e..f7cc4588 100644 --- a/src/modules/server/server.config.ts +++ b/src/modules/server/server.config.ts @@ -8,5 +8,5 @@ export class ServerConfig extends RedisConfig { @IsNumber() @Transform(({ value }) => parseInt(value)) - public WS_PORT!: number; + public WS_PORT = 3345; } From fa3f52ae98058d8fea67e8357ae7fb231c76a53b Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Tue, 1 Oct 2024 15:37:29 +0200 Subject: [PATCH 05/10] fixup! fix tests --- src/infra/auth-guard/config/x-api-key.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/infra/auth-guard/config/x-api-key.config.ts b/src/infra/auth-guard/config/x-api-key.config.ts index c8628096..a1ede92f 100644 --- a/src/infra/auth-guard/config/x-api-key.config.ts +++ b/src/infra/auth-guard/config/x-api-key.config.ts @@ -4,5 +4,5 @@ import { IsArray } from 'class-validator'; export class XApiKeyConfig { @Transform(({ value }) => value.split(',').map((part: string) => (part.split(':').pop() ?? '').trim())) @IsArray() - public ADMIN_API__ALLOWED_API_KEYS!: string[]; + public ADMIN_API__ALLOWED_API_KEYS = ['randomString']; } From d0035667a418b7daa447a40e7a96844df4757c71 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Wed, 2 Oct 2024 14:15:19 +0200 Subject: [PATCH 06/10] BC-8092 - code review --- .env.default | 6 +---- .env.test | 9 +++++++ src/infra/auth-guard/auth-guard.module.ts | 2 +- src/infra/auth-guard/config/index.ts | 1 - .../strategy/x-api-key.strategy.spec.ts | 2 +- .../auth-guard/strategy/x-api-key.strategy.ts | 2 +- .../{config => }/x-api-key.config.ts | 2 +- .../authorization/authorization.config.ts | 2 +- .../configuration/configuration.module.ts | 21 +++++++++++++-- .../configuration/configuration.service.ts | 14 +++++----- src/infra/redis/redis.config.ts | 8 +++--- src/infra/storage/storage.config.ts | 24 +++++++++++++++-- src/infra/storage/storage.service.ts | 27 ++++++------------- 13 files changed, 75 insertions(+), 45 deletions(-) create mode 100644 .env.test delete mode 100644 src/infra/auth-guard/config/index.ts rename src/infra/auth-guard/{config => }/x-api-key.config.ts (81%) diff --git a/.env.default b/.env.default index e5ae46a6..17def02d 100644 --- a/.env.default +++ b/.env.default @@ -1,13 +1,9 @@ -REDIS=redis://172.18.0.5:6379 -REDIS_PREFIX=y +REDIS=redis://localhost:6379 API_HOST=http://localhost:3030 ADMIN_API__ALLOWED_API_KEYS=randomString S3_ENDPOINT=localhost -S3_BUCKET=ydocs S3_PORT=9000 S3_SSL=false S3_ACCESS_KEY=miniouser S3_SECRET_KEY=miniouser - -WS_PORT="3345" \ No newline at end of file diff --git a/.env.test b/.env.test new file mode 100644 index 00000000..17def02d --- /dev/null +++ b/.env.test @@ -0,0 +1,9 @@ +REDIS=redis://localhost:6379 +API_HOST=http://localhost:3030 +ADMIN_API__ALLOWED_API_KEYS=randomString + +S3_ENDPOINT=localhost +S3_PORT=9000 +S3_SSL=false +S3_ACCESS_KEY=miniouser +S3_SECRET_KEY=miniouser diff --git a/src/infra/auth-guard/auth-guard.module.ts b/src/infra/auth-guard/auth-guard.module.ts index b0e674e2..fdb682f6 100644 --- a/src/infra/auth-guard/auth-guard.module.ts +++ b/src/infra/auth-guard/auth-guard.module.ts @@ -1,8 +1,8 @@ import { Module } from '@nestjs/common'; import { PassportModule } from '@nestjs/passport'; import { ConfigurationModule } from '../configuration/configuration.module.js'; -import { XApiKeyConfig } from './config/x-api-key.config.js'; import { XApiKeyStrategy } from './strategy/index.js'; +import { XApiKeyConfig } from './x-api-key.config.js'; @Module({ imports: [PassportModule, ConfigurationModule.register(XApiKeyConfig)], diff --git a/src/infra/auth-guard/config/index.ts b/src/infra/auth-guard/config/index.ts deleted file mode 100644 index 63c916bb..00000000 --- a/src/infra/auth-guard/config/index.ts +++ /dev/null @@ -1 +0,0 @@ -export * from './x-api-key.config.js'; diff --git a/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts b/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts index 4a3e9af1..7258545e 100644 --- a/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts +++ b/src/infra/auth-guard/strategy/x-api-key.strategy.spec.ts @@ -1,7 +1,7 @@ import { createMock } from '@golevelup/ts-jest'; import { UnauthorizedException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; -import { XApiKeyConfig } from '../config/x-api-key.config.js'; +import { XApiKeyConfig } from '../x-api-key.config.js'; import { XApiKeyStrategy } from './x-api-key.strategy.js'; describe('XApiKeyStrategy', () => { diff --git a/src/infra/auth-guard/strategy/x-api-key.strategy.ts b/src/infra/auth-guard/strategy/x-api-key.strategy.ts index 69640582..1fb5f85c 100644 --- a/src/infra/auth-guard/strategy/x-api-key.strategy.ts +++ b/src/infra/auth-guard/strategy/x-api-key.strategy.ts @@ -1,8 +1,8 @@ import { Injectable, UnauthorizedException } from '@nestjs/common'; import { PassportStrategy } from '@nestjs/passport'; import { Strategy } from 'passport-headerapikey/lib/Strategy.js'; -import { XApiKeyConfig } from '../config/index.js'; import { StrategyType } from '../interface/index.js'; +import { XApiKeyConfig } from '../x-api-key.config.js'; @Injectable() export class XApiKeyStrategy extends PassportStrategy(Strategy, StrategyType.API_KEY) { diff --git a/src/infra/auth-guard/config/x-api-key.config.ts b/src/infra/auth-guard/x-api-key.config.ts similarity index 81% rename from src/infra/auth-guard/config/x-api-key.config.ts rename to src/infra/auth-guard/x-api-key.config.ts index a1ede92f..c8628096 100644 --- a/src/infra/auth-guard/config/x-api-key.config.ts +++ b/src/infra/auth-guard/x-api-key.config.ts @@ -4,5 +4,5 @@ import { IsArray } from 'class-validator'; export class XApiKeyConfig { @Transform(({ value }) => value.split(',').map((part: string) => (part.split(':').pop() ?? '').trim())) @IsArray() - public ADMIN_API__ALLOWED_API_KEYS = ['randomString']; + public ADMIN_API__ALLOWED_API_KEYS!: string[]; } diff --git a/src/infra/authorization/authorization.config.ts b/src/infra/authorization/authorization.config.ts index 4fe13add..0e8cf560 100644 --- a/src/infra/authorization/authorization.config.ts +++ b/src/infra/authorization/authorization.config.ts @@ -2,5 +2,5 @@ import { IsUrl } from 'class-validator'; export class AuthorizationConfig { @IsUrl({ require_tld: false }) - public API_HOST = 'http://localhost:3030'; + public API_HOST!: string; } diff --git a/src/infra/configuration/configuration.module.ts b/src/infra/configuration/configuration.module.ts index 767df8a4..48507614 100644 --- a/src/infra/configuration/configuration.module.ts +++ b/src/infra/configuration/configuration.module.ts @@ -1,12 +1,29 @@ import { DynamicModule, Module } from '@nestjs/common'; -import { ConfigModule } from '@nestjs/config'; +import { ConfigModule, ConfigModuleOptions } from '@nestjs/config'; import { Configuration } from './configuration.service.js'; +const getEnvConfig = (): ConfigModuleOptions => { + const envConfig = { + envFilePath: '.env', + ignoreEnvFile: false, + }; + + if (process.env.NODE_ENV === 'test') { + envConfig.envFilePath = '.env.test'; + } + + if (process.env.NODE_ENV === 'production') { + envConfig.ignoreEnvFile = true; + } + + return envConfig; +}; + @Module({}) export class ConfigurationModule { public static register(Constructor: new () => T): DynamicModule { return { - imports: [ConfigModule.forRoot({ cache: true })], + imports: [ConfigModule.forRoot({ cache: true, ...getEnvConfig() })], providers: [ Configuration, { diff --git a/src/infra/configuration/configuration.service.ts b/src/infra/configuration/configuration.service.ts index 3e71fed0..405ae8f6 100644 --- a/src/infra/configuration/configuration.service.ts +++ b/src/infra/configuration/configuration.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; -import { plainToInstance } from 'class-transformer'; +import { plainToClassFromExist } from 'class-transformer'; import { validateSync } from 'class-validator'; @Injectable() @@ -8,8 +8,8 @@ export class Configuration { public constructor(private readonly configService: ConfigService) {} public getAllValidConfigsByType(Constructor: new () => T): T { - const config = new Constructor(); - const configKeys = Object.keys(config); + const configInstance = new Constructor(); + const configKeys = Object.keys(configInstance); const configValues = configKeys.reduce((acc: Record, key) => { const value = this.configService.get(key); @@ -21,14 +21,14 @@ export class Configuration { return acc; }, {}); - const validatedConfig = this.validate(configValues, Constructor); + const config = plainToClassFromExist(configInstance, configValues, { enableImplicitConversion: false }); + const validatedConfig = this.validate(config); return validatedConfig; } - private validate(config: Record, Constructor: new () => T): T { - const validatedConfig = plainToInstance(Constructor, config); - const errors = validateSync(validatedConfig); + private validate(validatedConfig: T): T { + const errors = validateSync(validatedConfig, { skipMissingProperties: false }); if (errors.length > 0) { throw new Error(errors.toString()); diff --git a/src/infra/redis/redis.config.ts b/src/infra/redis/redis.config.ts index 47692c55..9edad4c5 100644 --- a/src/infra/redis/redis.config.ts +++ b/src/infra/redis/redis.config.ts @@ -1,12 +1,12 @@ -import { IsOptional, IsString, IsUrl } from 'class-validator'; +import { IsString, IsUrl, ValidateIf } from 'class-validator'; export class RedisConfig { @IsUrl({ protocols: ['redis'], require_tld: false }) - @IsOptional() + @ValidateIf((o) => o.REDIS_SENTINEL_SERVICE_NAME === undefined) public REDIS!: string; @IsString() - @IsOptional() + @ValidateIf((o) => o.REDIS === undefined) public REDIS_SENTINEL_SERVICE_NAME!: string; @IsString() @@ -16,6 +16,6 @@ export class RedisConfig { public REDIS_SENTINEL_NAME = 'mymaster'; @IsString() - @IsOptional() + @ValidateIf((o) => o.REDIS_SENTINEL_SERVICE_NAME !== undefined) public REDIS_SENTINEL_PASSWORD!: string; } diff --git a/src/infra/storage/storage.config.ts b/src/infra/storage/storage.config.ts index 7a674ac3..5640766e 100644 --- a/src/infra/storage/storage.config.ts +++ b/src/infra/storage/storage.config.ts @@ -1,10 +1,30 @@ -import { IsOptional, IsString } from 'class-validator'; +import { Transform, Type } from 'class-transformer'; +import { IsBoolean, IsNumber, IsString } from 'class-validator'; export class StorageConfig { @IsString() - @IsOptional() public S3_ENDPOINT!: string; @IsString() public S3_BUCKET = 'ydocs'; + + @IsNumber() + @Type(() => Number) + public S3_PORT!: number; + + @IsBoolean() + @Transform(({ value }: { value: string }) => { + if (value.toLowerCase() === 'true') { + return true; + } else { + return false; + } + }) + public S3_SSL!: boolean; + + @IsString() + public S3_ACCESS_KEY!: string; + + @IsString() + public S3_SECRET_KEY!: string; } diff --git a/src/infra/storage/storage.service.ts b/src/infra/storage/storage.service.ts index ca943534..8b6d10bf 100644 --- a/src/infra/storage/storage.service.ts +++ b/src/infra/storage/storage.service.ts @@ -4,7 +4,6 @@ import { StorageConfig } from './storage.config.js'; @Injectable() export class StorageService { - private s3Endpoint: string; private bucketName: string; public constructor( @@ -12,29 +11,19 @@ export class StorageService { private logger: Logger, ) { this.logger.setContext(StorageService.name); - this.s3Endpoint = this.config.S3_ENDPOINT; this.bucketName = this.config.S3_BUCKET; } public async get(): Promise { - let store; + this.logger.log('using s3 store'); + // @ts-expect-error - @y/redis is only having jsdoc types + const { createS3Storage } = await import('@y/redis/storage/s3'); - if (this.s3Endpoint) { - this.logger.log('using s3 store'); - // @ts-expect-error - @y/redis is only having jsdoc types - const { createS3Storage } = await import('@y/redis/storage/s3'); - - store = createS3Storage(this.bucketName); - try { - // make sure the bucket exists - await store.client.makeBucket(this.bucketName); - } catch (e) {} - } else { - this.logger.log('ATTENTION! using in-memory store'); - // @ts-expect-error - @y/redis is only having jsdoc types - const { createMemoryStorage } = await import('@y/redis/storage/memory'); - store = createMemoryStorage(); - } + const store = createS3Storage(this.bucketName); + try { + // make sure the bucket exists + await store.client.makeBucket(this.bucketName); + } catch (e) {} return store; } From 7df809447eb2d8fb34d04ade10929ec691c132ed Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Mon, 14 Oct 2024 08:43:46 +0200 Subject: [PATCH 07/10] BC-8092 - fix sonercloud issues --- src/infra/storage/storage.service.ts | 6 +++--- src/modules/server/api/websocket.gateway.ts | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/infra/storage/storage.service.ts b/src/infra/storage/storage.service.ts index 8b6d10bf..5fe92d05 100644 --- a/src/infra/storage/storage.service.ts +++ b/src/infra/storage/storage.service.ts @@ -4,11 +4,11 @@ import { StorageConfig } from './storage.config.js'; @Injectable() export class StorageService { - private bucketName: string; + private readonly bucketName: string; public constructor( - private config: StorageConfig, - private logger: Logger, + private readonly config: StorageConfig, + private readonly logger: Logger, ) { this.logger.setContext(StorageService.name); this.bucketName = this.config.S3_BUCKET; diff --git a/src/modules/server/api/websocket.gateway.ts b/src/modules/server/api/websocket.gateway.ts index e7ab9e4a..e50e8706 100644 --- a/src/modules/server/api/websocket.gateway.ts +++ b/src/modules/server/api/websocket.gateway.ts @@ -14,12 +14,12 @@ export const UWS = 'UWS'; @Injectable() export class WebsocketGateway implements OnModuleInit, OnModuleDestroy { public constructor( - @Inject(UWS) private webSocketServer: TemplatedApp, - private storage: StorageService, - private authorizationService: AuthorizationService, - private redisService: RedisService, - private config: ServerConfig, - private logger: Logger, + @Inject(UWS) private readonly webSocketServer: TemplatedApp, + private readonly storage: StorageService, + private readonly authorizationService: AuthorizationService, + private readonly redisService: RedisService, + private readonly config: ServerConfig, + private readonly logger: Logger, ) { this.logger.setContext(WebsocketGateway.name); } From f400d3397a827a3bcdd5954d6d0935060df4a628 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Wed, 16 Oct 2024 12:22:20 +0200 Subject: [PATCH 08/10] Refactor configuration module and fix error handling in storage service --- src/infra/configuration/configuration.module.ts | 3 ++- src/infra/redis/redis.config.ts | 6 +++--- src/infra/storage/storage.service.ts | 4 +++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/infra/configuration/configuration.module.ts b/src/infra/configuration/configuration.module.ts index 48507614..23b60a69 100644 --- a/src/infra/configuration/configuration.module.ts +++ b/src/infra/configuration/configuration.module.ts @@ -4,6 +4,7 @@ import { Configuration } from './configuration.service.js'; const getEnvConfig = (): ConfigModuleOptions => { const envConfig = { + cache: true, envFilePath: '.env', ignoreEnvFile: false, }; @@ -23,7 +24,7 @@ const getEnvConfig = (): ConfigModuleOptions => { export class ConfigurationModule { public static register(Constructor: new () => T): DynamicModule { return { - imports: [ConfigModule.forRoot({ cache: true, ...getEnvConfig() })], + imports: [ConfigModule.forRoot(getEnvConfig())], providers: [ Configuration, { diff --git a/src/infra/redis/redis.config.ts b/src/infra/redis/redis.config.ts index 9edad4c5..f854687c 100644 --- a/src/infra/redis/redis.config.ts +++ b/src/infra/redis/redis.config.ts @@ -2,11 +2,11 @@ import { IsString, IsUrl, ValidateIf } from 'class-validator'; export class RedisConfig { @IsUrl({ protocols: ['redis'], require_tld: false }) - @ValidateIf((o) => o.REDIS_SENTINEL_SERVICE_NAME === undefined) + @ValidateIf((o: RedisConfig) => o.REDIS_SENTINEL_SERVICE_NAME === undefined) public REDIS!: string; @IsString() - @ValidateIf((o) => o.REDIS === undefined) + @ValidateIf((o: RedisConfig) => o.REDIS === undefined) public REDIS_SENTINEL_SERVICE_NAME!: string; @IsString() @@ -16,6 +16,6 @@ export class RedisConfig { public REDIS_SENTINEL_NAME = 'mymaster'; @IsString() - @ValidateIf((o) => o.REDIS_SENTINEL_SERVICE_NAME !== undefined) + @ValidateIf((o: RedisConfig) => o.REDIS_SENTINEL_SERVICE_NAME !== undefined) public REDIS_SENTINEL_PASSWORD!: string; } diff --git a/src/infra/storage/storage.service.ts b/src/infra/storage/storage.service.ts index 5fe92d05..06dcee4c 100644 --- a/src/infra/storage/storage.service.ts +++ b/src/infra/storage/storage.service.ts @@ -23,7 +23,9 @@ export class StorageService { try { // make sure the bucket exists await store.client.makeBucket(this.bucketName); - } catch (e) {} + } catch (e) { + this.logger.error(`Failed to create bucket ${this.bucketName}`, e); + } return store; } From aa65fc643fa30ccfaef0231328ff9500c6171c8f Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Wed, 16 Oct 2024 14:58:07 +0200 Subject: [PATCH 09/10] Refactor RedisConfig class and enable Redis cluster support --- .../tldraw-server/templates/configmap.yml.j2 | 1 + src/infra/redis/redis.config.ts | 20 ++- src/infra/redis/redis.service.spec.ts | 151 ++++++++++++------ src/infra/redis/redis.service.ts | 6 +- 4 files changed, 124 insertions(+), 54 deletions(-) diff --git a/ansible/roles/tldraw-server/templates/configmap.yml.j2 b/ansible/roles/tldraw-server/templates/configmap.yml.j2 index dc0baf20..83808b6e 100644 --- a/ansible/roles/tldraw-server/templates/configmap.yml.j2 +++ b/ansible/roles/tldraw-server/templates/configmap.yml.j2 @@ -11,4 +11,5 @@ data: WS_PORT: "3345" LOG: "@y/redis" FEATURE_PROMETHEUS_METRICS_ENABLED: "true" + REDIS_CLUSTER_ENABLED: "true" REDIS_SENTINEL_SERVICE_NAME: valkey-headless.{{ NAMESPACE }}.svc.cluster.local diff --git a/src/infra/redis/redis.config.ts b/src/infra/redis/redis.config.ts index f854687c..68c6851b 100644 --- a/src/infra/redis/redis.config.ts +++ b/src/infra/redis/redis.config.ts @@ -1,12 +1,24 @@ -import { IsString, IsUrl, ValidateIf } from 'class-validator'; +import { Transform } from 'class-transformer'; +import { IsBoolean, IsOptional, IsString, IsUrl, ValidateIf } from 'class-validator'; export class RedisConfig { + @IsBoolean() + @IsOptional() + @Transform(({ value }: { value: string }) => { + if (value.toLowerCase() === 'true') { + return true; + } else { + return false; + } + }) + public REDIS_CLUSTER_ENABLED!: boolean; + @IsUrl({ protocols: ['redis'], require_tld: false }) - @ValidateIf((o: RedisConfig) => o.REDIS_SENTINEL_SERVICE_NAME === undefined) + @ValidateIf((o: RedisConfig) => o.REDIS_CLUSTER_ENABLED === false) public REDIS!: string; @IsString() - @ValidateIf((o: RedisConfig) => o.REDIS === undefined) + @ValidateIf((o: RedisConfig) => o.REDIS_CLUSTER_ENABLED === true) public REDIS_SENTINEL_SERVICE_NAME!: string; @IsString() @@ -16,6 +28,6 @@ export class RedisConfig { public REDIS_SENTINEL_NAME = 'mymaster'; @IsString() - @ValidateIf((o: RedisConfig) => o.REDIS_SENTINEL_SERVICE_NAME !== undefined) + @ValidateIf((o: RedisConfig) => o.REDIS_CLUSTER_ENABLED === true) public REDIS_SENTINEL_PASSWORD!: string; } diff --git a/src/infra/redis/redis.service.spec.ts b/src/infra/redis/redis.service.spec.ts index 28ff35df..60afe6f6 100644 --- a/src/infra/redis/redis.service.spec.ts +++ b/src/infra/redis/redis.service.spec.ts @@ -17,57 +17,116 @@ describe('Redis Service', () => { }); describe('createRedisInstance', () => { - describe('when sentinelServiceName, sentinelName, sentinelPassword are set', () => { - describe('when resolveSrv resolves', () => { - const setup = () => { - const sentinelServiceName = 'serviceName'; - const sentinelName = 'sentinelName'; - const sentinelPassword = 'sentinelPassword'; - const redisPrefix = 'A'; - - const config = new RedisConfig(); - - config.REDIS = sentinelServiceName; - config.REDIS_SENTINEL_SERVICE_NAME = sentinelServiceName; - config.REDIS_PREFIX = redisPrefix; - config.REDIS_SENTINEL_NAME = sentinelName; - config.REDIS_SENTINEL_PASSWORD = sentinelPassword; - - const name1 = 'name1'; - const name2 = 'name2'; - const port1 = '11'; - const port2 = '22'; - const records = [ - { name: name1, port: port1 }, - { name: name2, port: port2 }, - ]; - const resolveSrv = jest.fn().mockResolvedValueOnce(records); - jest.spyOn(util, 'promisify').mockReturnValueOnce(resolveSrv); - - //const redisMock = createMock(); - //jest.spyOn(ioredisModule, 'Redis').mockReturnValueOnce(redisMock); - const logger = createMock(); - const service = new RedisService(config, logger); - - return { resolveSrv, sentinelServiceName, service }; - }; - - it('calls resolveSrv', async () => { - const { resolveSrv, sentinelServiceName, service } = setup(); - - await service.createRedisInstance(); - - expect(resolveSrv).toHaveBeenLastCalledWith(sentinelServiceName); - }); + describe('when REDIS_CLUSTER_ENABLED is true', () => { + const setup = () => { + const sentinelServiceName = 'serviceName'; + const sentinelName = 'sentinelName'; + const sentinelPassword = 'sentinelPassword'; + const redisPrefix = 'A'; + + const config = new RedisConfig(); + + config.REDIS = sentinelServiceName; + config.REDIS_CLUSTER_ENABLED = true; + config.REDIS_SENTINEL_SERVICE_NAME = sentinelServiceName; + config.REDIS_PREFIX = redisPrefix; + config.REDIS_SENTINEL_NAME = sentinelName; + config.REDIS_SENTINEL_PASSWORD = sentinelPassword; + + const name1 = 'name1'; + const name2 = 'name2'; + const port1 = 11; + const port2 = 22; + const records = [ + { name: name1, port: port1 }, + { name: name2, port: port2 }, + ]; + const resolveSrv = jest.fn().mockResolvedValueOnce(records); + jest.spyOn(util, 'promisify').mockReturnValueOnce(resolveSrv); + // @ts-ignore + const constructorSpy = jest.spyOn(Redis.prototype, 'constructor'); + + const logger = createMock(); + const service = new RedisService(config, logger); - it('creates a new Redis instance', async () => { - const { service } = setup(); + return { resolveSrv, sentinelServiceName, service, constructorSpy }; + }; + + it('calls resolveSrv', async () => { + const { resolveSrv, sentinelServiceName, service } = setup(); + + await service.createRedisInstance(); + + expect(resolveSrv).toHaveBeenLastCalledWith(sentinelServiceName); + }); - const redisInstance = await service.createRedisInstance(); + it('create new Redis instance with correctly props', async () => { + const { service, constructorSpy } = setup(); - expect(redisInstance).toBeInstanceOf(Redis); + await service.createRedisInstance(); + + expect(constructorSpy).toHaveBeenCalledWith({ + sentinels: [ + { host: 'name1', port: 11 }, + { host: 'name2', port: 22 }, + ], + sentinelPassword: 'sentinelPassword', + password: 'sentinelPassword', + name: 'sentinelName', }); }); + + it('creates a new Redis instance', async () => { + const { service } = setup(); + + const redisInstance = await service.createRedisInstance(); + + expect(redisInstance).toBeInstanceOf(Redis); + }); + }); + + describe('when REDIS_CLUSTER_ENABLED is false', () => { + const setup = () => { + const config = new RedisConfig(); + + config.REDIS = 'redis://localhost:6379'; + + const resolveSrv = jest.fn(); + jest.spyOn(util, 'promisify').mockReturnValueOnce(resolveSrv); + + const redisMock = createMock(); + // @ts-ignore + const constructorSpy = jest.spyOn(Redis.prototype, 'constructor'); + + const logger = createMock(); + const service = new RedisService(config, logger); + + return { resolveSrv, service, redisMock, constructorSpy }; + }; + + it('calls resolveSrv', async () => { + const { resolveSrv, service } = setup(); + + await service.createRedisInstance(); + + expect(resolveSrv).not.toHaveBeenCalled(); + }); + + it('create new Redis instance with correctly props', async () => { + const { service, constructorSpy } = setup(); + + await service.createRedisInstance(); + + expect(constructorSpy).toHaveBeenCalledWith('redis://localhost:6379'); + }); + + it('creates a new Redis instance', async () => { + const { service } = setup(); + + const redisInstance = await service.createRedisInstance(); + + expect(redisInstance).toBeInstanceOf(Redis); + }); }); }); }); diff --git a/src/infra/redis/redis.service.ts b/src/infra/redis/redis.service.ts index 37ddff84..57eb8a39 100644 --- a/src/infra/redis/redis.service.ts +++ b/src/infra/redis/redis.service.ts @@ -7,7 +7,6 @@ import { RedisConfig } from './redis.config.js'; @Injectable() export class RedisService { - private readonly sentinelServiceName: string; private readonly redisDeletionKey: string; private readonly redisDeletionActionKey: string; private internalRedisInstance?: Redis; @@ -18,7 +17,6 @@ export class RedisService { ) { const redisPrefix = this.config.REDIS_PREFIX; - this.sentinelServiceName = this.config.REDIS_SENTINEL_SERVICE_NAME; this.redisDeletionKey = `${redisPrefix}:delete`; this.redisDeletionActionKey = `${redisPrefix}:delete:action`; this.logger.setContext(RedisService.name); @@ -26,7 +24,7 @@ export class RedisService { public async createRedisInstance(): Promise { let redisInstance: Redis; - if (this.sentinelServiceName) { + if (this.config.REDIS_CLUSTER_ENABLED) { redisInstance = await this.createRedisSentinelInstance(); } else { redisInstance = this.createNewRedisInstance(); @@ -84,7 +82,7 @@ export class RedisService { private async discoverSentinelHosts(): Promise<{ host: string; port: number }[]> { const resolveSrv = util.promisify(dns.resolveSrv); try { - const records = await resolveSrv(this.sentinelServiceName); + const records = await resolveSrv(this.config.REDIS_SENTINEL_SERVICE_NAME); const hosts = records.map((record) => ({ host: record.name, From ea88874ff8969bdab0fbd25e883313884f2bf2a2 Mon Sep 17 00:00:00 2001 From: SevenWaysDP Date: Wed, 16 Oct 2024 15:15:08 +0200 Subject: [PATCH 10/10] Refactor error handling in StorageService and remove unnecessary logging --- src/infra/storage/storage.service.ts | 2 +- src/modules/worker/worker.service.ts | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/infra/storage/storage.service.ts b/src/infra/storage/storage.service.ts index 06dcee4c..478b16dd 100644 --- a/src/infra/storage/storage.service.ts +++ b/src/infra/storage/storage.service.ts @@ -24,7 +24,7 @@ export class StorageService { // make sure the bucket exists await store.client.makeBucket(this.bucketName); } catch (e) { - this.logger.error(`Failed to create bucket ${this.bucketName}`, e); + this.logger.log(e); } return store; diff --git a/src/modules/worker/worker.service.ts b/src/modules/worker/worker.service.ts index 157805ff..060f24b5 100644 --- a/src/modules/worker/worker.service.ts +++ b/src/modules/worker/worker.service.ts @@ -1,22 +1,22 @@ import { Injectable, OnModuleInit } from '@nestjs/common'; -import { ConfigService } from '@nestjs/config'; // @ts-expect-error - @y/redis is only having jsdoc types import { createWorker } from '@y/redis'; import { RedisService } from '../../infra/redis/redis.service.js'; import { StorageService } from '../../infra/storage/storage.service.js'; +import { WorkerConfig } from './worker.config.js'; @Injectable() export class WorkerService implements OnModuleInit { public constructor( - private storage: StorageService, - private redisService: RedisService, - private configService: ConfigService, + private readonly storage: StorageService, + private readonly redisService: RedisService, + private readonly config: WorkerConfig, ) {} public async onModuleInit(): Promise { await createWorker( await this.storage.get(), - this.configService.get('REDIS_PREFIX') ?? 'y', + this.config.REDIS_PREFIX, {}, this.redisService.createRedisInstance.bind(this.redisService), );