-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature/bull_mq_integration #2735
base: dev
Are you sure you want to change the base?
Changes from 3 commits
4632b91
faaacf5
44343e8
8e1ff70
00ec074
146d628
5b83c6e
606a006
63e6172
e7d416b
ab614af
dbf0374
a26bb97
216bd60
ef46137
b20a326
3193904
db1999f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -35,3 +35,6 @@ WEB_UI_SDK_URL=http://localhost:5202 | |||||||
#HASHING_KEY_SECRET="$2b$10$FovZTB91/QQ4Yu28nvL8e." | ||||||||
HASHING_KEY_SECRET_BASE64=JDJiJDEwJDNFeWtwWEs4QkdiczlRaWFwLkM4Vk8= | ||||||||
NOTION_API_KEY=secret | ||||||||
REDIS_HOST=localhost | ||||||||
REDIS_PORT=7381 | ||||||||
REDIS_PASSWORD=password | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security concern: Weak Redis password. The
Replace the current line with: -REDIS_PASSWORD=password
+REDIS_PASSWORD=<strong-unique-password> Also, add a comment above this line:
📝 Committable suggestion
Suggested change
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you create a new docker-compose file and didn't use the existing one? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
version: '3.8' | ||
services: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it located on a new docker image? Our service has to use redis in order to run ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why cant we use the same docker-compose-db.yml ? we can use same like this
Bring only redis using : This Line
|
||
redis: | ||
image: redis:alpine | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it for local only? Because the Alpine image is less performant than the Debian image There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes it's local only |
||
ports: | ||
- '${REDIS_PORT}:6379' | ||
volumes: | ||
- redis-data:/data | ||
command: > | ||
--requirepass ${REDIS_PASSWORD} | ||
--appendonly yes | ||
environment: | ||
- REDIS_PASSWORD=${REDIS_PASSWORD} | ||
- REDIS_PORT=${REDIS_PORT} | ||
networks: | ||
- app-network | ||
volumes: | ||
redis-data: | ||
driver: local | ||
driver_opts: | ||
type: tmpfs | ||
device: tmpfs | ||
o: size=100m | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reconsider volume configuration for data persistence. The current volume configuration uses a tmpfs (in-memory filesystem) with a 100MB limit. While this provides fast I/O, it has some potential drawbacks:
Consider the following alternatives:
Choose the option that best fits your persistence and performance requirements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good comment, i think "local" is the choice here for local development There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alonp99, glad you agree. I've noted that using (_/) ✏️ Learnings added
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this configuration? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the maximum size of the redis cache that can be set in your local computer under the file of tmp file system ( it will reset after restart ) |
||
networks: | ||
app-network: | ||
driver: bridge |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
"description": "workflow-service", | ||
"scripts": { | ||
"spellcheck": "cspell \"*\"", | ||
"setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run db:reset && npm run seed", | ||
"setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run docker:redis:down && npm run docker:redis && npm run db:reset && npm run seed", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is better
|
||
"format": "prettier --write . '!**/*.{md,hbs}'", | ||
"format:check": "prettier --check . '!**/*.{md,hbs}'", | ||
"lint": "eslint . --fix", | ||
|
@@ -30,6 +30,8 @@ | |
"db:reset:dev:with-data": "npm run db:reset:dev && npm run db:data-migration:migrate && npm run db:data-sync", | ||
"db:init": "npm run db:migrate-dev -- --name 'initial version' && npm run db:migrate-up seed", | ||
"prisma:generate": "prisma generate", | ||
"docker:redis": "docker compose -f docker-compose.redis.yml up -d --wait", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you run this command this way, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from what i checked it worked |
||
"docker:redis:down": "docker compose -f docker-compose.redis.yml down --volumes", | ||
"docker:db": "docker compose -f docker-compose.db.yml up -d --wait", | ||
"docker:db:down": "docker compose -f docker-compose.db.yml down --volumes", | ||
"docker:build": "docker build .", | ||
|
@@ -50,8 +52,12 @@ | |
"@ballerine/common": "0.9.33", | ||
"@ballerine/workflow-core": "0.6.45", | ||
"@ballerine/workflow-node-sdk": "0.6.45", | ||
"@bull-board/api": "^6.0.0", | ||
"@bull-board/express": "^6.0.0", | ||
"@bull-board/nestjs": "^6.0.0", | ||
"@faker-js/faker": "^7.6.0", | ||
"@nestjs/axios": "^2.0.0", | ||
"@nestjs/bullmq": "^10.2.1", | ||
"@nestjs/common": "^9.3.12", | ||
"@nestjs/config": "2.3.1", | ||
"@nestjs/core": "^9.3.12", | ||
|
@@ -78,6 +84,7 @@ | |
"ballerine-nestjs-typebox": "3.0.2-next.11", | ||
"base64-stream": "^1.0.0", | ||
"bcrypt": "5.1.0", | ||
"bullmq": "^5.13.2", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should also include redis client There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add bull-arena which is the UI component for bull |
||
"class-transformer": "0.5.1", | ||
"class-validator": "0.14.0", | ||
"concat-stream": "^2.0.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import { Module } from '@nestjs/common'; | ||
import { ConfigModule } from '@nestjs/config'; | ||
import { BullModule } from '@nestjs/bullmq'; | ||
import { BullBoardModule } from '@bull-board/nestjs'; | ||
import { ExpressAdapter } from '@bull-board/express'; | ||
import { BullAdapter } from '@bull-board/api/bullAdapter'; | ||
import { env } from '@/env'; | ||
import { WebhookService } from '@/bull-mq/webhook/webhook.service'; | ||
import { AppLoggerService } from '@/common/app-logger/app-logger.service'; | ||
import { WebhookProcessor } from '@/bull-mq/webhook/webhook.processor'; | ||
import { AppLoggerModule } from '@/common/app-logger/app-logger.module'; | ||
|
||
const QUEUE_NAMES = [ | ||
{ | ||
name: 'webhook-queue', | ||
config: {}, | ||
}, | ||
]; | ||
|
||
@Module({ | ||
imports: [ | ||
AppLoggerModule, | ||
ConfigModule.forRoot({ isGlobal: true }), | ||
BullModule.forRootAsync({ | ||
useFactory: () => ({ | ||
connection: { | ||
password: env.REDIS_PASSWORD, | ||
host: env.REDIS_HOST, | ||
port: env.REDIS_PORT, | ||
}, | ||
}), | ||
}), | ||
BullModule.registerQueue(...QUEUE_NAMES.map(({ name }) => ({ name }))), | ||
BullBoardModule.forRoot({ | ||
route: '/queues', | ||
adapter: ExpressAdapter, | ||
}), | ||
...QUEUE_NAMES.map(({ name }) => | ||
BullBoardModule.forFeature({ | ||
name, | ||
adapter: BullAdapter, | ||
}), | ||
), | ||
], | ||
providers: [WebhookProcessor, WebhookService, AppLoggerService], | ||
exports: [BullModule, WebhookService], | ||
}) | ||
export class BullMqModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { InjectQueue, Processor, OnQueueEvent } from '@nestjs/bullmq'; | ||
import { Job, Queue } from 'bullmq'; | ||
import axios, { AxiosRequestConfig, Method } from 'axios'; | ||
import { AppLoggerService } from '@/common/app-logger/app-logger.service'; | ||
|
||
export interface WebhookJobData { | ||
url: string; | ||
method: Method; | ||
headers?: Record<string, string>; | ||
body?: Record<string, unknown>; | ||
timeout?: number; | ||
} | ||
|
||
@Processor('webhook-queue') | ||
export class WebhookProcessor { | ||
constructor( | ||
protected readonly logger: AppLoggerService, | ||
@InjectQueue('webhook-queue') private webhookQueue: Queue, | ||
) {} | ||
|
||
async process(job: Job<WebhookJobData>) { | ||
this.logger.log(`Processing webhook job ${job.id}`); | ||
|
||
const { url, method, headers, body, timeout } = job.data; | ||
|
||
const config: AxiosRequestConfig = { | ||
url, | ||
method, | ||
headers, | ||
data: body, | ||
timeout: timeout || 5000, // Default timeout of 5 seconds | ||
}; | ||
|
||
try { | ||
const response = await axios(config); | ||
|
||
return { | ||
status: response.status, | ||
statusText: response.statusText, | ||
headers: response.headers, | ||
data: response.data, | ||
}; | ||
} catch (error) { | ||
if (axios.isAxiosError(error)) { | ||
throw new Error(`Webhook request failed: ${error.message}`); | ||
} | ||
|
||
throw error; | ||
} | ||
} | ||
|
||
@OnQueueEvent('completed') | ||
onCompleted(job: Job) { | ||
this.logger.log(`Webhook job ${job.id} completed successfully`); | ||
} | ||
|
||
@OnQueueEvent('failed') | ||
onFailed(job: Job, error: Error) { | ||
this.logger.error(`Webhook job ${job.id} failed: ${error.message}`); | ||
} | ||
|
||
async onModuleInit() { | ||
this.webhookQueue.on('cleaned', (jobs, type) => { | ||
this.logger.log(`${jobs.length} ${type} jobs have been cleaned from the webhook queue`); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
import { InjectQueue } from '@nestjs/bullmq'; | ||
import { Queue } from 'bullmq'; | ||
import { WebhookJobData } from './webhook.processor'; // You might want to move this interface to a separate file | ||
|
||
@Injectable() | ||
export class WebhookService { | ||
constructor(@InjectQueue('webhook-queue') private webhookQueue: Queue) {} | ||
|
||
async addWebhookJob(data: WebhookJobData) { | ||
await this.webhookQueue.add('webhook', data); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,9 @@ export const serverEnvSchema = { | |
IN_MEMORIES_SECRET_ACQUIRER_ID: z.string().optional(), | ||
IN_MEMORIES_SECRET_PRIVATE_KEY: z.string().optional(), | ||
IN_MEMORIES_SECRET_CONSUMER_KEY: z.string().optional(), | ||
REDIS_HOST: z.string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance the While adding
Here's a suggested implementation: REDIS_HOST: z.string().describe('Redis server hostname or IP address'), If you want to add stricter validation, you could use a regex pattern: REDIS_HOST: z
.string()
.regex(/^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$/)
.describe('Redis server hostname or IP address'), This regex pattern allows for valid hostnames and IP addresses. |
||
REDIS_PASSWORD: z.string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Make The current implementation makes
Here's a suggested implementation: REDIS_PASSWORD: z.string().optional().describe('Redis server password (if required)'), This change allows for Redis configurations without authentication while still supporting those that require a password. |
||
REDIS_PORT: z.string().transform(value => Number(value)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance The current implementation transforms the port to a number but lacks validation. Consider adding the following improvements:
Here's a suggested implementation: REDIS_PORT: z
.string()
.default('6379')
.transform(value => {
const port = Number(value);
if (isNaN(port) || port < 1024 || port > 65535) {
throw new Error('REDIS_PORT must be a number between 1024 and 65535');
}
return port;
})
.describe('Redis server port'), This implementation adds a description, includes port number validation, and provides a default value of 6379 (the standard Redis port). |
||
}; | ||
|
||
if (!process.env['ENVIRONMENT_NAME'] || process.env['ENVIRONMENT_NAME'] === 'local') { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would want something like USE_REDIS - to change between using redis to im-mem implmenation