feature/bull_mq_integration #2735
Conversation
|
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 214 files out of 300 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe changes in this pull request introduce several new environment variables for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/docker-compose.redis.yml (1)
3-16: Approve Redis service configuration with a minor suggestion.The Redis service configuration is well-structured and follows good practices:
- Using the Alpine-based image for a smaller footprint
- Exposing the port via an environment variable for flexibility
- Mounting a volume for data persistence
- Setting a password for security
- Enabling append-only mode for data durability
- Using a custom network for isolation
However, there's a minor redundancy in the configuration.
Consider removing the redundant environment variables:
environment: - - REDIS_PASSWORD=${REDIS_PASSWORD} - - REDIS_PORT=${REDIS_PORT}These variables are already used in the configuration and don't need to be explicitly set in the environment section.
services/workflows-service/package.json (1)
8-8: LGTM! Consider adding a wait for Redis.The addition of
npm run docker:redisto the setup script is a good change, ensuring Redis is started as part of the setup process. This aligns well with the PR objectives of introducing Redis defaults.Consider adding a wait for Redis to be ready, similar to how you wait for the database:
- "setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run docker:redis && 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 && wait-on tcp:6379 && npm run db:reset && npm run seed",This ensures that Redis is fully ready before proceeding with subsequent steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- services/workflows-service/.env.example (1 hunks)
- services/workflows-service/docker-compose.redis.yml (1 hunks)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/src/app.module.ts (2 hunks)
- services/workflows-service/src/env.ts (1 hunks)
🔇 Additional comments (11)
services/workflows-service/docker-compose.redis.yml (2)
1-1: LGTM: Docker Compose version is appropriate.The chosen Docker Compose version (3.8) is suitable for modern Docker environments and provides access to recent features.
24-26: LGTM: Network configuration is appropriate.The custom bridge network 'app-network' is well-defined and follows best practices:
- It provides isolation for the Redis service.
- The configuration is simple and suitable for most use cases.
This setup allows for easy communication between services while maintaining network separation from other Docker networks.
services/workflows-service/.env.example (1)
38-40:⚠️ Potential issueImprove Redis configuration for security and consistency
While adding Redis configuration is a good step for integrating BullMQ, there are a few points to consider:
Security: Using 'password' as the default Redis password in the .env.example file is a security risk. Consider using a placeholder like '' instead.
Configuration:
- The Redis port (7381) is non-standard. If there's no specific reason for this, consider using the default Redis port 6379.
- 'local' as the host might be confusing. Consider using 'localhost' or '127.0.0.1' for clarity.
Consistency: To maintain consistency with other database configurations in this file, consider using uppercase for the host (e.g., 'LOCALHOST' instead of 'local').
Here's a suggested improvement:
-REDIS_HOST=local -REDIS_PORT=7381 -REDIS_PASSWORD=password +REDIS_HOST=LOCALHOST +REDIS_PORT=6379 +REDIS_PASSWORD=<your-redis-password>To ensure these Redis settings are used consistently across the project, run the following script:
This script will help identify where the Redis configuration is used and if there are any hardcoded Redis connections that should be using these environment variables instead.
✅ Verification successful
Redis configuration verified and recommendation applied
The Redis configurations are correctly utilized across the project without any hardcoded connections, ensuring security best practices are followed.
Recommendation:
Update the
.env.exampleto use placeholders for sensitive information and standardize configuration values:-REDIS_HOST=local -REDIS_PORT=7381 -REDIS_PASSWORD=password +REDIS_HOST=LOCALHOST +REDIS_PORT=6379 +REDIS_PASSWORD=<your-redis-password>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Redis configuration usage across the project # Search for Redis configuration usage echo "Searching for Redis configuration usage:" rg --type-add 'config:*.{js,ts,json,yaml,yml}' --type config 'REDIS_(HOST|PORT|PASSWORD)' # Check for any hardcoded Redis configurations echo "Checking for hardcoded Redis configurations:" rg --type-add 'code:*.{js,ts}' --type code 'redis.(createClient|connect)\('Length of output: 1142
services/workflows-service/src/env.ts (2)
93-98: Summary of Redis configuration changesThe addition of Redis-related environment variables is a positive step towards integrating Redis with the workflows service. The implementation is generally sound, with room for minor improvements in validation and flexibility.
Key points:
- New variables added:
REDIS_HOST,REDIS_PASSWORD, andREDIS_PORT.- Suggestions made for enhancing validation and flexibility.
- Verification needed to ensure proper usage of these variables across the project.
Overall, these changes lay the groundwork for Redis integration, which aligns with the PR objective of introducing enhancements related to Redis defaults within the project.
93-98: Verify the usage of new Redis environment variablesThe addition of Redis configuration variables suggests that Redis integration is being added to the workflows service. However, there are no other changes visible in this file or mentioned in the context that show how these variables will be used.
To ensure these variables are properly utilized, please run the following script to check for their usage across the project:
This script will help verify that the new environment variables are being used appropriately in the project and that Redis is being properly integrated.
services/workflows-service/src/app.module.ts (2)
51-51: LGTM: BullModule import added correctly.The import statement for BullModule from '@nestjs/bullmq' is correctly placed and uses the appropriate package for NestJS integration with BullMQ.
130-136: BullModule configuration looks good, but consider some improvements.The BullModule configuration is correctly placed and uses environment variables for Redis connection details, which is a good practice. However, there are a few points to consider:
Consider adding default values or validation for
REDIS_PORTandREDIS_PASSWORD. This will prevent potential issues if these environment variables are not set.It's recommended to add error handling for the Redis connection to avoid silent failures. You could wrap the BullModule configuration in a try-catch block or use a custom provider to handle connection errors.
Ensure that these environment variables (REDIS_HOST, REDIS_PORT, REDIS_PASSWORD) are properly documented and set in your deployment environments.
Here's a suggested improvement:
BullModule.forRootAsync({ useFactory: (configService: ConfigService) => ({ connection: { host: configService.get('REDIS_HOST', 'localhost'), port: configService.get('REDIS_PORT', 6379), password: configService.get('REDIS_PASSWORD'), }, }), inject: [ConfigService], }),Let's verify the usage of these environment variables:
services/workflows-service/package.json (4)
33-34: Great addition of Redis management scripts!The new
docker:redisanddocker:redis:downscripts are well-implemented:
- They use a separate Docker Compose file, promoting modularity.
- The
--waitflag ensures the Redis service is ready before the command completes.- The
--volumesflag in the down command ensures proper cleanup.These scripts align well with the PR objectives and provide convenient commands for managing the Redis service.
57-57: Excellent choice for Redis-based queue integration!The addition of
@nestjs/bullmqis a great choice for integrating Redis-based queues with NestJS. This aligns perfectly with the PR objectives of improving Redis integration and enhancing message queuing capabilities.
Line range hint
1-184: Overall, excellent changes for Redis and BullMQ integration!The modifications to
package.jsoneffectively support the PR objectives:
- The setup script now includes Redis initialization.
- New scripts for managing the Redis service have been added.
- Dependencies for @nestjs/bullmq and bullmq have been included.
These changes provide a solid foundation for integrating Redis and improving message queuing in the workflows service. Great job on maintaining consistency and following best practices!
84-84: Good addition of BullMQ core library.The inclusion of
bullmqis necessary and complements the@nestjs/bullmqdependency well. This further supports the PR objectives of enhancing Redis integration and message queuing capabilities.Please ensure that the versions of
@nestjs/bullmqandbullmqare compatible. You can verify this by checking the peer dependencies of@nestjs/bullmq:If there are any version mismatches, consider updating the versions to ensure compatibility.
| volumes: | ||
| redis-data: | ||
| driver: local | ||
| driver_opts: | ||
| type: tmpfs | ||
| device: tmpfs | ||
| o: size=100m |
There was a problem hiding this comment.
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:
- Data volatility: All data will be lost when the container stops.
- Size limitation: 100MB might be restrictive depending on your data volume.
Consider the following alternatives:
-
For true persistence, use a named volume without tmpfs:
volumes: redis-data: driver: local
-
If tmpfs is required for performance, consider increasing the size limit and be aware of the data loss risk:
volumes: redis-data: driver: local driver_opts: type: tmpfs device: tmpfs o: size=1g # Adjust size as needed
-
For a balance of persistence and performance, consider using the local driver with a bind mount to an SSD on the host:
volumes: redis-data: driver: local driver_opts: type: none o: bind device: /path/on/host/redis-data
Choose the option that best fits your persistence and performance requirements.
There was a problem hiding this comment.
Good comment, i think "local" is the choice here for local development
There was a problem hiding this comment.
@alonp99, glad you agree. I've noted that using driver: local is the choice for local development.
(_/)
(•ᴥ•)
/ 🍃
✏️ Learnings added
Learnt from: alonp99
PR: ballerine-io/ballerine#2735
File: services/workflows-service/docker-compose.redis.yml:17-23
Timestamp: 2024-09-30T07:58:41.803Z
Learning: For Redis volumes in local development, prefer using `driver: local` in Docker Compose configurations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| REDIS_HOST: z.string(), | ||
| REDIS_PASSWORD: z.string(), | ||
| REDIS_PORT: z | ||
| .string() | ||
| .default('6379') | ||
| .transform(value => Number(value)), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing Redis configuration variables
The addition of Redis-related environment variables is a good step towards integrating Redis with the workflows service. However, consider the following improvements:
- Add validation for
REDIS_PORTto ensure it's within a valid range (e.g., 1024-65535). - Make
REDIS_PASSWORDoptional, as not all Redis configurations require authentication. - Add comments explaining the purpose of these new variables for better maintainability.
Here's a suggested implementation:
// Redis configuration for BullMQ integration
REDIS_HOST: z.string().describe('Redis server hostname'),
REDIS_PASSWORD: z.string().optional().describe('Redis server password (if required)'),
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 descriptions, makes the password optional, and includes port number validation.
| version: '3.8' | ||
| services: | ||
| redis: | ||
| image: redis:alpine |
There was a problem hiding this comment.
Is it for local only? Because the Alpine image is less performant than the Debian image
| driver_opts: | ||
| type: tmpfs | ||
| device: tmpfs | ||
| o: size=100m |
There was a problem hiding this comment.
Can you explain this configuration?
There was a problem hiding this comment.
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 )
this is not really an issue - as it is a queue system...
in any matter we don't want to keep a lot of information in our open source system at our customers
There was a problem hiding this comment.
Why did you create a new docker-compose file and didn't use the existing one?
| @@ -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 && npm run db:reset && npm run seed", | |||
There was a problem hiding this comment.
Don't you need to run docker:redis:down as well?
| @@ -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.
If you run this command this way, the REDIS_PORT variable won't be loaded from the dotenv. Is it part of the requirements?
There was a problem hiding this comment.
from what i checked it worked
| REDIS_PASSWORD: z.string(), | ||
| REDIS_PORT: z | ||
| .string() | ||
| .default('6379') |
There was a problem hiding this comment.
Does it make sense to set it as the default?
| .default('6379') | |
| .default('7381') |
|
Can i share with you some bull services that might help you to work with their queues? |
| @@ -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=local | |||
| @@ -0,0 +1,26 @@ | |||
| version: '3.8' | |||
| services: | |||
There was a problem hiding this comment.
Why is it located on a new docker image? Our service has to use redis in order to run ...
you can place it with a dependency and when Redis is ready start workflow service
There was a problem hiding this comment.
Why cant we use the same docker-compose-db.yml ?
we can use same like this
version: '3'
services:
db:
image: sibedge/postgres-plv8:15.3-3.1.7
ports:
- ${DB_PORT}:5432
environment:
POSTGRES_USER: ${DB_USER}
POSTGRES_PASSWORD: ${DB_PASSWORD}
volumes:
- postgres15:/var/lib/postgresql/data
redis:
image: redis:alpine
ports:
- '${REDIS_PORT}:6379'
volumes:
- redis-data:/data
command: >
--requirepass ${REDIS_PASSWORD}
--appendonly yes
environment:
- REDIS_PASSWORD=${REDIS_PASSWORD}
- REDIS_PORT=${REDIS_PORT}
volumes:
postgres15: ~
redis-data:
driver: local
Bring only redis using :
This Line
can be replaced with the following
docker:redis: docker compose -f docker-compose.db.yml up -d redis --wait
docker:redis:down: docker compose -f docker-compose.db.yml down -v redis,
| @@ -78,6 +81,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.
you should also include redis client
| @@ -126,6 +127,13 @@ export const validate = async (config: Record<string, unknown>) => { | |||
| RuleEngineModule, | |||
| NotionModule, | |||
| SecretsManagerModule, | |||
| BullModule.forRoot({ | |||
| connection: { | |||
There was a problem hiding this comment.
its wrong way to init a module in NestJS also i would recommend also to use: config.get('redis')
in case we would like to include more options like ENFORCE SSL or any other configuration parameter which supported by redis client
| @@ -78,6 +81,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.
add bull-arena which is the UI component for bull
| BullModule.forRoot({ | ||
| connection: { | ||
| host: env.REDIS_HOST || 'localhost', | ||
| port: env.REDIS_PORT, | ||
| password: env.REDIS_PASSWORD, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
BullModule.forRootAsync({
imports: [ConfigModule],
useFactory: async (configService: ConfigService) => ({
redis: configService.get('redis'),
settings: {
maxStalledCount: 3,
stalledInterval: 30_000,
guardInterval: 5000,
},
}),
inject: [ConfigService],
}),
```
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
services/workflows-service/src/bull-mq/webhook/webhook.service.ts (2)
1-4: Consider moving WebhookJobData interface to a separate fileThe comment on line 4 suggests moving the
WebhookJobDatainterface to a separate file. This is a good practice for better code organization and reusability.Consider creating a new file (e.g.,
webhook.types.ts) and moving theWebhookJobDatainterface there. Then update the import statement accordingly.
1-13: Overall implementation looks good with minor suggestionsThe
WebhookServiceimplementation using NestJS and BullMQ looks correct and follows best practices. Here's a summary of the review:
- Consider moving the
WebhookJobDatainterface to a separate file for better code organization.- Verify that the queue name 'webhook-queue' is consistently used in your BullMQ configuration.
- Ensure that the job name 'webhook' matches the name expected by your webhook processor.
Additionally, consider the following improvements:
- Add error handling in the
addWebhookJobmethod to catch and log any errors that might occur when adding a job to the queue.- Consider adding options to the
Queue.add()method call, such as priority or delay, if needed for your use case.Here's a suggested improvement for error handling:
async addWebhookJob(data: WebhookJobData) { try { await this.webhookQueue.add('webhook', data); } catch (error) { // Log the error or handle it as appropriate for your application console.error('Failed to add webhook job:', error); throw error; // Re-throw if you want calling code to handle it } }services/workflows-service/src/bull-mq/bull-mq.module.ts (5)
1-18: LGTM! Consider using an enum for queue names.The imports are comprehensive and relevant to the module's functionality. The
QUEUE_NAMESconstant is a good approach for defining queues.Consider using an enum for queue names to improve type safety and maintainability:
enum QueueName { WEBHOOK = 'webhook-queue', } const QUEUE_NAMES = [ { name: QueueName.WEBHOOK, config: {}, }, ];
24-33: LGTM! Consider adding error handling for Redis connection.The BullModule configuration is well-structured and follows best practices by using environment variables for connection details.
Consider adding error handling for the Redis connection:
BullModule.forRootAsync({ useFactory: () => { if (!env.REDIS_PASSWORD || !env.REDIS_HOST || !env.REDIS_PORT) { throw new Error('Redis configuration is incomplete'); } return { connection: { password: env.REDIS_PASSWORD, host: env.REDIS_HOST, port: env.REDIS_PORT, }, }; }, }),
34-43: LGTM! Consider adding authentication to the Bull Board route.The BullBoardModule configuration is well-structured and provides a good way to monitor queues.
Consider adding authentication to the Bull Board route to prevent unauthorized access to queue information:
BullBoardModule.forRoot({ route: '/queues', adapter: ExpressAdapter, middleware: [authMiddleware], // Add your authentication middleware here }),
45-48: LGTM! Consider using a constant for exported services.The module providers and exports are well-defined and appropriate for the module's functionality.
For consistency and maintainability, consider using a constant for exported services:
const EXPORTED_SERVICES = [BullModule, WebhookService] as const; @Module({ // ... other configurations providers: [WebhookProcessor, WebhookService, AppLoggerService], exports: EXPORTED_SERVICES, }) export class BullMqModule {}
1-48: Great implementation of BullMqModule! Consider the suggested improvements.The BullMqModule is well-structured and follows NestJS best practices. It successfully integrates BullMQ for job and queue management, sets up Bull Board for monitoring, and provides necessary services. The use of environment variables for configuration enhances security and flexibility.
To further improve the module:
- Consider using an enum for queue names.
- Add error handling for Redis connection configuration.
- Implement authentication for the Bull Board route.
- Use a constant for exported services for consistency.
These changes will enhance type safety, error handling, security, and maintainability of the module.
As this module is crucial for job processing and queue management, ensure that it's properly integrated with other parts of the application. Consider creating a separate configuration file for BullMQ-related settings to keep the module file cleaner and more focused on its primary responsibilities.
services/workflows-service/src/bull-mq/webhook/webhook.processor.ts (2)
21-50: LGTM with a minor suggestion: Process method is well-implemented.The
processmethod effectively handles webhook job processing, including proper extraction of job data, setting a default timeout, and returning relevant response data on success. The error handling is good, distinguishing between Axios errors and other errors.Consider adding more context to the error message for non-Axios errors:
- throw error; + throw new Error(`Unexpected error processing webhook: ${error.message}`);This will provide more informative error messages for debugging purposes.
62-67: LGTM with a minor suggestion: Module initialization is well-implemented.The
onModuleInitmethod effectively sets up a listener for the 'cleaned' event on the queue, providing useful logging information about queue maintenance operations.Consider adding error handling to the event listener:
async onModuleInit() { - this.webhookQueue.on('cleaned', (jobs, type) => { + this.webhookQueue.on('cleaned', (jobs, type) => { this.logger.log(`${jobs.length} ${type} jobs have been cleaned from the webhook queue`); }); + this.webhookQueue.on('error', (error) => { + this.logger.error(`Error in webhook queue: ${error.message}`); + }); }This will ensure that any errors in the queue are logged, improving the overall robustness of the system.
services/workflows-service/package.json (1)
8-8: LGTM with a minor suggestion.The addition of Redis setup commands in the "setup" script is consistent with the PR objectives. However, consider adding a wait command for Redis similar to the database setup to ensure Redis is fully operational before proceeding.
You could add a wait command for Redis like this:
- "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", + "setup": "npm run docker:db:down && npm run docker:db && wait-on tcp:5432 && npm run docker:redis:down && npm run docker:redis && wait-on tcp:6379 && npm run db:reset && npm run seed",This ensures that Redis is fully operational before proceeding with the rest of the setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
- services/workflows-service/.env.example (1 hunks)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/src/app.module.ts (3 hunks)
- services/workflows-service/src/bull-mq/bull-mq.module.ts (1 hunks)
- services/workflows-service/src/bull-mq/webhook/webhook.processor.ts (1 hunks)
- services/workflows-service/src/bull-mq/webhook/webhook.service.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/.env.example
- services/workflows-service/src/app.module.ts
- services/workflows-service/src/env.ts
🔇 Additional comments (10)
services/workflows-service/src/bull-mq/webhook/webhook.service.ts (3)
6-7: Class declaration looks goodThe
WebhookServiceclass is correctly decorated with@Injectable(), following NestJS best practices for dependency injection.
10-12: Method implementation looks good, verify job nameThe
addWebhookJobmethod correctly adds a job to the queue using async/await syntax. The job name 'webhook' is used, which is good.Please ensure that 'webhook' matches the job name expected by your webhook processor. Run the following script to verify:
#!/bin/bash # Description: Verify the job name 'webhook' is consistently used across the codebase. # Test: Search for 'webhook' job name in all TypeScript files rg --type typescript -A 5 $'\.add\([\'"]webhook[\'"]'
8-8: Constructor looks good, verify queue nameThe constructor correctly uses dependency injection for the BullMQ Queue. The queue name 'webhook-queue' is used, which is good.
Please ensure that 'webhook-queue' matches the queue name defined in your BullMQ configuration. Run the following script to verify:
✅ Verification successful
Please use the corrected script below to verify that
'webhook-queue'is consistently used across the codebase:
Queue name 'webhook-queue' is consistent across the codebase
The queue name
'webhook-queue'is correctly and consistently used in all relevant parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the queue name 'webhook-queue' is consistently used across the codebase. # Test: Search for 'webhook-queue' in all TypeScript files rg --type typescript -A 5 $'webhook-queue'Length of output: 93
Script:
#!/bin/bash # Description: Verify the queue name 'webhook-queue' is consistently used across the codebase. # Corrected Test: Search for 'webhook-queue' in all TypeScript files using the correct file type identifier. rg --type ts -A 5 'webhook-queue'Length of output: 1995
services/workflows-service/src/bull-mq/webhook/webhook.processor.ts (3)
1-12: LGTM: Imports and interface definition are well-structured.The imports cover all necessary dependencies, and the
WebhookJobDatainterface is well-defined with appropriate properties for configuring webhook jobs. The optionaltimeoutproperty provides flexibility in job configuration.
14-19: LGTM: Class definition and constructor follow NestJS best practices.The
WebhookProcessorclass is correctly decorated with@Processor('webhook-queue'), and the constructor uses dependency injection for the logger and queue. This approach aligns with NestJS best practices and promotes maintainability.
52-60: LGTM: Event handlers are well-implemented.The
onCompletedandonFailedevent handlers are correctly decorated with@OnQueueEventand provide appropriate logging for job outcomes. These handlers will be useful for monitoring and debugging the webhook processing system.services/workflows-service/package.json (4)
33-34: LGTM! Addresses previous review comment.The addition of "docker:redis" and "docker:redis:down" scripts is consistent with the PR objectives. The "docker:redis:down" script properly addresses the concern raised in the previous review comment by MatanYadaev.
87-87: LGTM! Completes BullMQ integration.The addition of the "bullmq" dependency completes the set of required packages for BullMQ integration. The version specified (^5.13.2) allows for minor and patch updates, which is a good practice for keeping the package up-to-date while avoiding potentially breaking changes.
Line range hint
1-190: Overall assessment: LGTM with minor suggestionsThe changes to
package.jsoneffectively integrate Redis and BullMQ into the project, aligning well with the PR objectives. The additions include:
- Updated setup script to manage Redis alongside the database.
- New scripts for Docker-based Redis management.
- New dependencies for BullMQ and Bull Board integration.
These changes provide a solid foundation for implementing job queues and monitoring in the workflow service. Consider the minor suggestions provided in the individual comments to further improve the setup process and dependency management.
55-57: LGTM! Addresses previous review comments partially.The addition of BullMQ and Bull Board dependencies is consistent with the PR objectives. The inclusion of Bull Board packages suggests that a UI for managing Bull queues will be implemented, which is a good practice for monitoring and managing job queues.
Regarding the past review comments:
The Redis client is not explicitly added, but it might be included as a sub-dependency of BullMQ. Let's verify this:
For the suggestion to add bull-arena, it seems that @bull-board has been chosen instead. This is a valid alternative that provides similar functionality.
Also applies to: 60-60
| @@ -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 | |||
There was a problem hiding this comment.
I think we would want something like USE_REDIS - to change between using redis to im-mem implmenation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
services/workflows-service/src/bull-mq/webhook/types/types.ts (1)
3-9: LGTM: Well-structured WebhookJobData interface with minor suggestions.The
WebhookJobDatainterface is well-defined and covers the essential properties for a webhook job. Here are a few suggestions for potential improvements:
Consider using a more specific type for the
bodyproperty instead ofRecord<string, unknown>. This could help prevent potential runtime errors by ensuring the correct data structure at compile-time.It might be beneficial to add a comment or consider using a custom type for the
timeoutproperty to ensure it's always a positive number.Here's an example of how you could implement these suggestions:
// You might want to define a more specific type for the body type WebhookBody = { // Define your expected body structure here }; export interface WebhookJobData { url: string; method: Method; headers?: Record<string, string>; body?: WebhookBody; // Ensure timeout is always positive timeout?: number & { __brand: 'PositiveNumber' }; } // Helper function to create a positive number function createPositiveNumber(value: number): number & { __brand: 'PositiveNumber' } { if (value <= 0) throw new Error('Timeout must be a positive number'); return value as number & { __brand: 'PositiveNumber' }; }This approach provides more type safety and helps prevent potential runtime errors.
services/workflows-service/src/bull-mq/consts.ts (2)
1-1: Consider using a more generic import path.The current import statement is using a specific path to the ESM distribution file. While this works, it might be more robust to use a more generic import path.
Consider changing the import to:
-import { BaseJobOptions } from 'bullmq/dist/esm/interfaces'; +import { BaseJobOptions } from 'bullmq';This change would make the import less dependent on the specific package structure and potentially more resilient to future updates of the
bullmqpackage.
3-14: Approve the QUEUES constant structure with a suggestion for enhancement.The
QUEUESconstant is well-structured and uses appropriate type validation. The configuration forINCOMING_WEBHOOKS_QUEUEseems suitable for handling incoming webhooks with a reasonable retry strategy.To enhance flexibility, consider extracting the queue configuration into a separate constant. This would allow easier modification of queue settings and potential reuse. For example:
const INCOMING_WEBHOOKS_QUEUE_CONFIG = { name: 'webhook-queue', config: { attempts: 10, backoff: { type: 'exponential' as const, delay: 1000, }, }, }; export const QUEUES = { INCOMING_WEBHOOKS_QUEUE: INCOMING_WEBHOOKS_QUEUE_CONFIG, } satisfies Record<string, { name: string; config: BaseJobOptions }>;This structure would make it easier to add more queues in the future and allow for easier testing and modification of individual queue configurations.
services/workflows-service/src/bull-mq/bull-mq.module.ts (1)
15-29: BullModule configuration looks good, but consider adding error handling.The BullModule configuration correctly uses environment variables for Redis connection details, which is secure and flexible. Dynamically registering queues based on the QUEUES constant is an efficient approach.
Consider adding error handling for missing environment variables. Here's a suggested implementation:
BullModule.forRootAsync({ useFactory: () => { if (!env.REDIS_PASSWORD || !env.REDIS_HOST || !env.REDIS_PORT) { throw new Error('Missing Redis configuration in environment variables'); } return { connection: { password: env.REDIS_PASSWORD, host: env.REDIS_HOST, port: env.REDIS_PORT, }, }; }, }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- services/workflows-service/docker-compose.redis.yml (1 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/bull-mq/bull-mq.module.ts (1 hunks)
- services/workflows-service/src/bull-mq/consts.ts (1 hunks)
- services/workflows-service/src/bull-mq/webhook/types/types.ts (1 hunks)
- services/workflows-service/src/bull-mq/webhook/webhook.processor.ts (1 hunks)
- services/workflows-service/src/bull-mq/webhook/webhook.service.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🚧 Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/docker-compose.redis.yml
- services/workflows-service/src/bull-mq/webhook/webhook.processor.ts
- services/workflows-service/src/bull-mq/webhook/webhook.service.ts
🔇 Additional comments (4)
services/workflows-service/src/bull-mq/webhook/types/types.ts (1)
1-1: LGTM: Appropriate import for webhook method types.The import of
Methodfrom 'axios' is correct and necessary for defining the HTTP method types in theWebhookJobDatainterface.services/workflows-service/src/bull-mq/consts.ts (1)
1-14: Overall, the implementation is solid with room for minor enhancements.The
QUEUESconstant provides a clear and type-safe way to define queue configurations. The use of BullMQ for job processing aligns well with the PR objectives of integrating with Redis and improving message queuing. The exponential backoff strategy for job retries is a good choice for handling potential transient issues.To further improve this implementation:
- Consider the suggestion to use a more generic import path for
bullmq.- Think about extracting the queue configuration into a separate constant for increased flexibility.
These changes would enhance the maintainability and scalability of the queue configuration system.
services/workflows-service/src/bull-mq/bull-mq.module.ts (2)
41-44: LGTM! Module providers and exports are well-defined.The providers (WebhookProcessor and WebhookService) and exports (BullModule and WebhookService) are appropriately set up for the module's functionality. This configuration allows other modules to use the configured queues and webhook functionality.
1-10: LGTM! Imports look appropriate.The imports cover all necessary modules and services for BullMQ integration. Using environment variables for configuration is a good practice.
Let's verify the contents of the QUEUES constant:
✅ Verification successful
Let's try a different approach to locate the
QUEUESconstant.
Let's retrieve the full definition of the
QUEUESconstant for further verification.
QUEUES Constant Verified Successfully
The
QUEUESconstant inconsts.tsis correctly defined with theINCOMING_WEBHOOKS_QUEUEand appropriate configuration settings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the QUEUES constant # Test: Search for the QUEUES constant definition rg --type typescript -A 10 'export const QUEUES ='Length of output: 87
Script:
#!/bin/bash # Description: Verify the contents of the QUEUES constant by searching all TypeScript files. grep -r 'export const QUEUES =' --include='*.ts' .Length of output: 127
Script:
#!/bin/bash # Description: Get the full definition of the QUEUES constant. grep -A 20 'export const QUEUES =' --include='*.ts' ./services/workflows-service/src/bull-mq/consts.tsLength of output: 376
| BullBoardModule.forRoot({ | ||
| route: '/queues', | ||
| adapter: ExpressAdapter, | ||
| }), | ||
| ...Object.values(QUEUES).map(queue => | ||
| BullBoardModule.forFeature({ | ||
| name: queue.name, | ||
| adapter: BullAdapter, | ||
| }), | ||
| ), |
There was a problem hiding this comment.
💡 Codebase verification
'/queues' route is not secured.
The '/queues' route in BullBoardModule is currently exposed without authentication or authorization measures. It's recommended to secure this route to prevent unauthorized access to the queue monitoring interface.
- Implement authentication middleware for the '/queues' route.
- Review and enhance global security configurations to ensure they protect all sensitive routes.
🔗 Analysis chain
BullBoardModule configuration is correct, but consider security implications.
The BullBoardModule is properly configured for monitoring queues, using the correct adapters and dynamically adding all queues from the QUEUES constant.
However, the '/queues' route might need to be secured in a production environment. Let's verify if there are any security measures in place:
If no security measures are found, consider adding authentication middleware to protect the '/queues' route.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security measures on the '/queues' route
# Test: Search for middleware or guards applied to the '/queues' route
rg --type typescript -A 5 "route: '/queues'"
# Test: Search for any global guards or middleware in the main application file
rg --type typescript -A 10 'const app = await NestFactory.create'
Length of output: 190
Script:
#!/bin/bash
# Description: Check for security measures on the '/queues' route
# Test: Search for middleware or guards applied to the '/queues' route
rg "route: '/queues'" -A 5 -g "*.ts" -g "*.tsx"
# Test: Search for any global guards or middleware in the main application file
rg "const app = await NestFactory.create" -A 10 -g "*.ts" -g "*.tsx"
Length of output: 2919
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (13)
services/workflows-service/src/bull-mq/outgoing-webhook/types/types.ts (2)
3-3: LGTM: Well-defined type alias with a suggestion for improvement.The
WebhookJobDatatype alias is well-defined, leveraging TypeScript's utility types to extract the parameter type from theinvokeWebhookmethod. This approach ensures type consistency and reduces duplication, which is a good practice.Consider adding a brief JSDoc comment to explain the purpose and usage of this type alias. For example:
/** Represents the data structure for a webhook job, matching the parameter of OutgoingWebhooksService.invokeWebhook */ export type WebhookJobData = Parameters<OutgoingWebhooksService['invokeWebhook']>[0];
1-3: Overall, excellent implementation with clear purpose and good practices.This file demonstrates a focused and efficient approach to type definition. It leverages TypeScript's advanced type system to create a reusable type that ensures consistency with the
OutgoingWebhooksService. The use of utility types and derived types is a best practice that reduces the chance of type inconsistencies as the codebase evolves.As the project grows, consider creating a central types file or directory for shared types like this, especially if
WebhookJobDatais used across multiple modules. This can help maintain a single source of truth for common types and improve overall code organization.services/workflows-service/src/webhooks/outgoing-webhooks/outgoing-webhooks.module.ts (2)
5-9: LGTM: Module declaration is well-structured. Consider adding a brief comment.The module declaration follows NestJS best practices, correctly importing, providing, and exporting the necessary components.
Consider adding a brief comment above the @module decorator to describe the purpose of this module:
/** * Module for handling outgoing webhooks. * Provides and exports OutgoingWebhooksService for managing webhook operations. */ @Module({ // ... (existing code) })
1-10: Overall assessment: Well-implemented NestJS module.This new OutgoingWebhooksModule is cleanly implemented, following NestJS best practices and conventions. It correctly imports dependencies, provides and exports the OutgoingWebhooksService, and maintains a clear structure. The module is well-positioned to handle outgoing webhook functionality within the application.
As the project grows, consider the following architectural points:
- Ensure that the OutgoingWebhooksService remains focused on outgoing webhook logic to maintain the Single Responsibility Principle.
- If the webhook functionality becomes more complex, consider creating separate modules for different types of webhooks or webhook-related operations.
- Monitor the usage of AppLoggerModule and ensure it provides adequate logging for webhook operations, especially for debugging and monitoring purposes in a production environment.
services/workflows-service/src/business-report/business-report.module.ts (1)
Line range hint
1-35: Consider addressing circular dependenciesThe file contains multiple imports with
eslint-disable-next-line import/no-cyclecomments, including the newly added AlertModule. These indicate circular dependencies between modules, which can lead to initialization issues and make the codebase harder to maintain.Consider refactoring the module structure to eliminate these circular dependencies. Some strategies include:
- Creating a shared module for common functionality.
- Using interfaces instead of concrete implementations where possible.
- Employing the mediator pattern to decouple modules.
Would you like assistance in developing a plan to address these circular dependencies?
services/workflows-service/src/webhooks/incoming/incoming-webhooks.service.ts (1)
Line range hint
22-115: Consider refactoringhandleIndividualAmlHitfor improved maintainabilityWhile not directly related to the class name change, the
handleIndividualAmlHitmethod is quite complex and handles multiple responsibilities. Consider the following improvements:
- Break down the method into smaller, more focused methods to improve readability and maintainability.
- Enhance error handling, particularly for database operations.
- Address the type mismatch indicated by the
@ts-expect-errorcomment to improve type safety.Here's a suggested refactoring approach:
async handleIndividualAmlHit({ endUserId, data }: { endUserId: string; data: IndividualAmlWebhookInput['data'] }) { this.logger.log('Started handling individual AML hit', { endUserId }); const endUser = await this.getEndUser(endUserId); const config = await this.getCustomerConfig(endUser.projectId); const workflowDefinitionId = await this.getWorkflowDefinitionId(config, endUser.projectId); const amlHits = this.processAmlHits(data); await this.updateEndUserAmlHits(endUserId, amlHits); if (amlHits.length === 0) { this.logger.log('No AML hits found', { endUserId }); return; } const workflow = await this.createOrUpdateWorkflow(workflowDefinitionId, endUser, data); this.logger.log(`Created workflow for AML hits`, { workflow }); } // Implement the following methods: // - getEndUser(endUserId: string): Promise<EndUser> // - getCustomerConfig(projectId: string): Promise<CustomerConfig> // - getWorkflowDefinitionId(config: CustomerConfig, projectId: string): Promise<string> // - processAmlHits(data: IndividualAmlWebhookInput['data']): AmlHit[] // - updateEndUserAmlHits(endUserId: string, amlHits: AmlHit[]): Promise<void> // - createOrUpdateWorkflow(workflowDefinitionId: string, endUser: EndUser, data: IndividualAmlWebhookInput['data']): Promise<Workflow>This refactoring improves readability, makes the code more modular, and allows for better error handling in each sub-method.
services/workflows-service/src/env.ts (1)
93-99: Overall assessment of Redis and queue system integrationThe additions of Redis-related environment variables (
REDIS_HOST,REDIS_PASSWORD,REDIS_PORT) and theQUEUE_SYSTEM_ENABLEDflag are good steps towards integrating Redis and potentially BullMQ into the workflow service. These changes provide the necessary configuration options for connecting to a Redis instance and controlling the queue system's activation.However, there are opportunities to enhance these additions:
- Add descriptions to all new variables for better maintainability.
- Implement stricter validation, especially for
REDIS_PORT.- Make
REDIS_PASSWORDoptional to support various Redis configurations.- Use more explicit typing for
QUEUE_SYSTEM_ENABLED.These improvements will make the configuration more robust, self-documenting, and flexible for different deployment scenarios.
Consider the following architectural advice:
- Ensure that the Redis connection is properly handled in the application code, including connection pooling and error handling.
- Implement a fallback mechanism or graceful degradation if Redis or the queue system is unavailable.
- Plan for potential future scalability by considering Redis cluster configurations.
- Document the purpose and usage of the queue system in the project documentation to guide other developers on when and how to use it.
services/workflows-service/src/app.module.ts (1)
50-52: LGTM! Consider updating documentation for the new modules.The addition of
BullMqModule,IncomingWebhooksModule, andOutgoingWebhooksModuleimproves the modularity of the application, especially for webhook handling. This aligns well with the PR objectives for enhancing Redis integration and message queuing.Consider updating the project documentation to reflect these new modules and their purposes, particularly the split between incoming and outgoing webhooks.
services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1)
7-7: Consider specifying a more precise generic type parameter instead ofany.Using
anyas the default for the generic typeTreduces type safety. Consider requiring subclasses to provide a specific type to enhance type checking and prevent potential runtime errors.services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts (1)
15-20: Implement the job processing logic inhandleJobThe
handleJobmethod contains a TODO comment indicating that the implementation for processing incoming webhook jobs is pending. Completing this method is essential to ensure that incoming webhooks are handled correctly.Would you like assistance in implementing the logic to process the incoming webhook data? I can help generate code to handle the webhook job internally or open a GitHub issue to track this task.
services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts (2)
48-48: Unused destructured variablesenvironmentandapiVersionThe variables
environmentandapiVersionare destructured fromwebhookbut are not used in thesendWebhookmethod. Consider removing them to avoid unused variables and potential confusion.
Line range hint
85-89: Potential exposure of sensitive data in error logsLogging
dataandwebhookin the error handling may expose sensitive information. Consider sanitizing or omitting sensitive details from the logs to prevent possible PII leakage.🧰 Tools
🪛 Biome
[error] 84-84: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
services/workflows-service/src/webhooks/incoming/incoming-webhooks.module.ts (1)
Line range hint
6-6: Fix typographical error in import pathThere's a typo in the import path for
WorkflowDefinitionModule:import { WorkflowDefinitionModule } from '@/workflow-defintion/workflow-definition.module';The directory
'@/workflow-defintion'should be'@/workflow-definition'.Apply this diff to correct the typo:
-import { WorkflowDefinitionModule } from '@/workflow-defintion/workflow-definition.module'; +import { WorkflowDefinitionModule } from '@/workflow-definition/workflow-definition.module';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- services/workflows-service/src/alert/alert.module.ts (3 hunks)
- services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts (2 hunks)
- services/workflows-service/src/app.module.ts (3 hunks)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/bull-mq.module.ts (1 hunks)
- services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/outgoing-webhook/types/types.ts (1 hunks)
- services/workflows-service/src/business-report/business-report.module.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
- services/workflows-service/src/redis/const/redis-config.ts (1 hunks)
- services/workflows-service/src/redis/redis.module.ts (1 hunks)
- services/workflows-service/src/webhooks/incoming/incoming-webhooks.controller.ts (2 hunks)
- services/workflows-service/src/webhooks/incoming/incoming-webhooks.module.ts (3 hunks)
- services/workflows-service/src/webhooks/incoming/incoming-webhooks.service.ts (1 hunks)
- services/workflows-service/src/webhooks/outgoing-webhooks/outgoing-webhooks.module.ts (1 hunks)
- services/workflows-service/src/webhooks/outgoing-webhooks/outgoing-webhooks.service.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/src/redis/redis.module.ts
🧰 Additional context used
🔇 Additional comments (31)
services/workflows-service/src/bull-mq/outgoing-webhook/types/types.ts (1)
1-1: LGTM: Import statement is correct and follows good practices.The import statement correctly imports the
OutgoingWebhooksServiceusing a path alias, which is a good practice for maintaining clean and readable import paths.services/workflows-service/src/redis/const/redis-config.ts (1)
1-7: Crucial configuration for Redis integrationThis configuration file is a key component in achieving the PR objective of introducing Redis defaults and enhancing the integration with Redis. It provides a flexible way to configure the Redis connection while allowing for environment-specific settings.
Ensure that this configuration is properly utilized in the BullMQ integration mentioned in the PR summary. Also, consider documenting the required environment variables in the project's README or configuration guide to assist other developers in setting up the Redis connection correctly.
services/workflows-service/src/webhooks/outgoing-webhooks/outgoing-webhooks.module.ts (2)
1-3: LGTM: Imports are well-structured and follow NestJS conventions.The imports are correctly organized, using absolute paths where appropriate. This approach enhances code readability and maintainability.
10-10: LGTM: Module export is correct.The OutgoingWebhooksModule is properly exported, allowing it to be imported and used in other parts of the application.
services/workflows-service/src/business-report/business-report.module.ts (1)
12-12: AlertModule import addedThe AlertModule has been added to the imports of the BusinessReportModule. This change appears to be intentional and aligns with the PR objectives of enhancing the project's functionality.
To ensure this addition is used correctly, let's verify its usage:
services/workflows-service/src/webhooks/incoming/incoming-webhooks.controller.ts (5)
2-2: LGTM: Import statement reorganization.The separate import for
BadRequestExceptionimproves code readability without affecting functionality.
Line range hint
1-58: Overall assessment: Refactoring improves code organization.The changes in this file primarily involve renaming and restructuring to better separate concerns between incoming and outgoing webhooks. The main functionality of the controller remains intact, which is a positive aspect of this refactoring.
Key points:
- The controller and service have been renamed to specifically handle incoming webhooks.
- Import statements have been updated to reflect the new structure.
- The core logic in the
amlHookmethod remains unchanged.These changes improve code organization and maintainability. However, it's crucial to ensure that these refactoring changes are consistently applied across the entire codebase to prevent any potential issues.
6-6: Verify the new import path for AmlWebhookInput.The import path change from './dtos/aml-webhook-input' to '../dtos/aml-webhook-input' suggests a project structure reorganization. While this change looks good, it's crucial to ensure that the file exists at the new location to prevent potential runtime errors.
✅ Verification successful
Import path for AmlWebhookInput is correct.
The new import path '../dtos/aml-webhook-input' has been verified, and the file exists at this location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the AmlWebhookInput file fd -p "src/webhooks/dtos/aml-webhook-input.ts"Length of output: 112
22-24: Verify consistency of IncomingWebhooksController usage across the codebase.The class renaming from
WebhooksControllertoIncomingWebhooksControllerand the updated service injection in the constructor align well with the refactoring to separate incoming and outgoing webhooks. These changes improve code clarity by better reflecting the controller's specific responsibility.However, it's crucial to ensure that these changes are consistently applied throughout the codebase to prevent any potential runtime errors or inconsistencies.
✅ Verification successful
Refactoring Verified Successfully
All references to
WebhooksControllerandWebhooksServicehave been updated toIncomingWebhooksControllerandIncomingWebhooksServicerespectively. No remaining references to the old names were found, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of IncomingWebhooksController across the codebase rg --type typescript "IncomingWebhooksController" # Verify there are no remaining references to the old WebhooksController rg --type typescript "WebhooksController" # Check for any remaining references to WebhooksService rg --type typescript "WebhooksService"Length of output: 235
Script:
#!/bin/bash # Verify the usage of IncomingWebhooksController across the codebase rg --type ts "IncomingWebhooksController" # Verify there are no remaining references to the old WebhooksController rg --type ts "WebhooksController" # Check for any remaining references to WebhooksService rg --type ts "WebhooksService"Length of output: 3307
8-8: Verify the implementation of IncomingWebhooksService.The import statement has been updated to use
IncomingWebhooksServiceinstead ofWebhooksService, which aligns with the service renaming. This change looks good, but it's important to ensure that theIncomingWebhooksServiceis correctly implemented and contains all the necessary methods used in this controller.✅ Verification successful
Please run the following scripts to verify the
IncomingWebhooksServiceimplementation:
IncomingWebhooksService Implementation Verified
The
IncomingWebhooksServiceis correctly implemented and contains thehandleIndividualAmlHitmethod as required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and implementation of IncomingWebhooksService fd -p "src/webhooks/incoming/incoming-webhooks.service.ts" ast-grep --lang typescript --pattern $'class IncomingWebhooksService { $$$ handleIndividualAmlHit($$$) { $$$ } $$$ }'Length of output: 235
Script:
#!/bin/bash # Verify the existence of IncomingWebhooksService fd 'incoming-webhooks.service.ts' src/webhooks/incoming/ # Verify the class and handleIndividualAmlHit method in IncomingWebhooksService ast-grep --lang typescript --pattern $'class IncomingWebhooksService { $$$ handleIndividualAmlHit($$$) { $$$ } $$$ }'Length of output: 11726
services/workflows-service/src/webhooks/incoming/incoming-webhooks.service.ts (2)
Line range hint
1-115: Summary of review
- The class name change from
WebhooksServicetoIncomingWebhooksServiceis approved and improves clarity.- A verification script has been provided to check for any references to the old class name in the codebase.
- While not directly related to the changes, suggestions for refactoring the
handleIndividualAmlHitmethod have been provided to improve code maintainability and readability.Overall, the changes look good, but please ensure to verify the impact on the rest of the codebase and consider the refactoring suggestions for future improvements.
11-11: Approve class name change and verify dependenciesThe change from
WebhooksServicetoIncomingWebhooksServiceimproves clarity and aligns well with the file name and the service's purpose. This change enhances code readability and maintainability.To ensure this change doesn't break existing code, please run the following script to check for any references to the old class name:
If any results are found, please update those references to use
IncomingWebhooksService.✅ Verification successful
Verified class name change and dependencies
No references to the old class name
WebhooksServicewere found outside ofOutgoingWebhooksService. The change toIncomingWebhooksServicedoes not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to the old class name 'WebhooksService' # Test: Search for 'WebhooksService' in TypeScript and JavaScript files rg --type-add 'script:*.{ts,js}' --type script 'WebhooksService' # Test: Search for 'webhooks.service' in TypeScript and JavaScript files (common import pattern) rg --type-add 'script:*.{ts,js}' --type script 'webhooks\.service'Length of output: 3555
services/workflows-service/src/alert/alert.module.ts (2)
27-28: LGTM: New module imports added correctly.The addition of
BullMqModuleandOutgoingWebhooksModuleimports aligns with the PR objectives of enhancing Redis integration and improving webhook functionality. These modules will likely provide necessary functionality for job queue management and outgoing webhook handling.
37-38: LGTM: New modules correctly added to imports array.The addition of
BullMqModuleandOutgoingWebhooksModuleto the imports array is consistent with the new import statements and allows the AlertModule to utilize their functionality.services/workflows-service/src/env.ts (1)
95-95: 🛠️ Refactor suggestionEnhance
REDIS_PORTwith validation, default value, and descriptionThe current implementation of
REDIS_PORTcan be improved. Consider the following enhancements:
- Add validation to ensure the port is within a valid range (1024-65535).
- Provide a default value (typically 6379 for Redis).
- Add a description for better maintainability.
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 default value, includes port number validation, and provides a description.
To verify if the default Redis port (6379) is already in use, you can run the following script:
services/workflows-service/src/app.module.ts (2)
90-91: LGTM! Modular approach to webhook handling.The addition of
IncomingWebhooksModuleandOutgoingWebhooksModuleto the imports array is consistent with the new imports and enhances the modularity of webhook handling in the application.
132-132: LGTM! Verify BullMQ configuration.The addition of
BullMqModuleto the imports array is consistent with the new import and aligns with the PR objectives for improving Redis integration and message queuing.Please ensure that the BullMQ configuration is properly set up, preferably using
config.get('redis')for Redis configuration as suggested in previous comments. You can verify this by checking theBullMqModuleimplementation:If the configuration is not set up as expected, consider updating it to use the ConfigService for better maintainability and consistency with other parts of the application.
services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (2)
1-5: Imports are correctly defined.All necessary modules are imported, and the paths appear to be correct. This sets up the required dependencies for the class.
40-42: Graceful shutdown is properly implemented inonModuleDestroy.The
onModuleDestroymethod correctly closes both the worker and the queue, ensuring that resources are cleaned up when the module is destroyed. This helps prevent memory leaks and other issues related to lingering connections.services/workflows-service/src/bull-mq/bull-mq.module.ts (1)
12-45: Well-structuredBullMqModuleImplementationThe module is correctly set up to integrate BullMQ and Bull Board with NestJS. It dynamically registers all queues from the
QUEUESconstant, properly configures the Redis connection usingREDIS_CONFIG, and integrates Bull Board for queue monitoring.services/workflows-service/src/alert/webhook-manager/webhook-manager.service.ts (4)
9-12: Imports are appropriateThe newly added imports (
AnyRecord,env,OutgoingWebhookQueueService,OutgoingWebhooksService) are necessary for the updated functionality and are correctly included.
23-24: Updated constructor dependenciesThe constructor now injects
OutgoingWebhookQueueServiceandOutgoingWebhooksService, replacing previous dependencies. This change aligns with the new architecture using queue services for webhook handling.
50-63: Queue system integration logicThe condition
if (env.QUEUE_SYSTEM_ENABLED)correctly checks whether the queue system is enabled. The implementation usingoutgoingQueueWebhookService.addJobadds the webhook to the queue as expected.
65-83: Direct webhook invocation fallbackWhen the queue system is not enabled, the code properly falls back to invoking the webhook directly using
outgoingWebhookService.invokeWebhook. The response handling and error throwing are appropriately managed.services/workflows-service/src/webhooks/incoming/incoming-webhooks.module.ts (7)
29-29: Import IncomingWebhooksService correctlyThe import statement properly references
IncomingWebhooksServicefrom the new path.
38-38: Import IncomingWebhooksController correctlyThe import statement correctly brings in
IncomingWebhooksControllerfrom the updated location.
41-41: Update controllers to use IncomingWebhooksControllerThe
controllersarray now appropriately includesIncomingWebhooksController, reflecting the focus on incoming webhooks.
79-79: Add IncomingWebhooksService to providersIncluding
IncomingWebhooksServicein theprovidersarray ensures that it can be injected where needed.
82-82: Export IncomingWebhooksService for external useExporting
IncomingWebhooksServiceallows other modules to access and utilize its functionality.
84-84: Rename module to IncomingWebhooksModuleRenaming the module to
IncomingWebhooksModuleaccurately reflects its specialized purpose.
84-84: Verify all references to WebhooksModule are updatedSince the module has been renamed, please ensure that all imports and references to
WebhooksModulethroughout the codebase are updated toIncomingWebhooksModule.Run the following script to confirm:
✅ Verification successful
All references to
WebhooksModulehave been successfully updated toIncomingWebhooksModule.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to 'WebhooksModule' have been updated. # Test: Search for 'WebhooksModule' in the codebase, excluding the current file. Expect: No occurrences. rg 'WebhooksModule' --type=typescript --glob '!services/workflows-service/src/webhooks/incoming/incoming-webhooks.module.ts'Length of output: 159
Script:
#!/bin/bash # Description: Verify that all references to 'WebhooksModule' have been updated. # Test: Search for 'WebhooksModule' in the codebase, excluding the current file. Expect: No occurrences. rg 'WebhooksModule' --type=ts --glob '!services/workflows-service/src/webhooks/incoming/incoming-webhooks.module.ts'Length of output: 1209
| import { env } from '@/env'; | ||
|
|
||
| export const REDIS_CONFIG = { | ||
| password: env.REDIS_PASSWORD || 'password', | ||
| host: env.REDIS_HOST || 'localhost', | ||
| port: env.REDIS_PORT || 7381, | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Enhance Security and Configuration Robustness
The Redis configuration is well-structured, but there are several areas that require attention:
-
Missing Environment Variables in
.env.exampleFiles:- The following
.env.examplefiles are missingREDIS_PASSWORD,REDIS_HOST, andREDIS_PORT:apps/kyb-app/.env.exampleapps/workflows-dashboard/.env.exampleservices/websocket-service/.env.example
- The following
-
Insecure Fallback Password:
- The fallback password
'password'is not secure for production use.
- The fallback password
-
Non-Standard Redis Port:
- The default port
7381is non-standard for Redis (typically6379).
- The default port
-
Lack of Type Checking for Port Number:
- There's no type checking to ensure the port number is a valid integer.
Consider the following improvements:
-
Remove the Fallback Password and Make It a Required Environment Variable:
- password: env.REDIS_PASSWORD || 'password', + password: env.REDIS_PASSWORD,
-
Use the Standard Redis Port as the Fallback:
- port: env.REDIS_PORT || 7381, + port: Number(env.REDIS_PORT) || 6379,
-
Add Type Checking for the Port Number:
- port: env.REDIS_PORT || 6379, + port: Number(env.REDIS_PORT) || 6379,
-
Ensure All
.env.exampleFiles Include Required Redis Environment Variables:- Add the following lines to each
.env.examplefile:REDIS_PASSWORD= REDIS_HOST=localhost REDIS_PORT=6379
- Add the following lines to each
🔗 Analysis chain
Enhance security and configuration robustness
The Redis configuration is well-structured, but there are a few areas for improvement:
- The fallback password 'password' is not secure for production use.
- The default port 7381 is non-standard for Redis (usually 6379).
- There's no type checking for the port number.
Consider the following improvements:
- Remove the fallback password and make it a required environment variable:
- password: env.REDIS_PASSWORD || 'password',
+ password: env.REDIS_PASSWORD,- Use the standard Redis port as the fallback:
- port: env.REDIS_PORT || 7381,
+ port: env.REDIS_PORT || 6379,- Add type checking for the port:
- port: env.REDIS_PORT || 6379,
+ port: Number(env.REDIS_PORT) || 6379,To ensure these environment variables are properly documented, let's check the .env.example file:
Additionally, to verify the uniqueness of the Redis port across the project:
Further Verification Required
The previous verification steps encountered issues. To ensure accurate analysis, please execute the following scripts:
[/run_scripts]
Further Verification Required
To ensure all .env.example files include REDIS_PASSWORD, REDIS_HOST, and REDIS_PORT, please execute the following scripts:
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Redis environment variables in .env.example
# Expected: REDIS_PASSWORD, REDIS_HOST, and REDIS_PORT should be present
grep -E "REDIS_(PASSWORD|HOST|PORT)" .env.example
Length of output: 97
Script:
#!/bin/bash
# Description: Check for other occurrences of the non-standard Redis port
# Expected: No other occurrences of port 7381 for Redis
rg --type=yaml --type=typescript --type=javascript "7381"
Length of output: 92
Script:
#!/bin/bash
# Description: Locate all .env files in the repository
# Expected: List of .env files present
fd --type f "^\.env" || find . -name ".env*"
Length of output: 453
Script:
#!/bin/bash
# Description: Search for occurrences of port 7381 in YAML, JavaScript, and TypeScript files
# Expected: List of files containing '7381'
rg "7381" --glob "*.yaml" --glob "*.yml" --glob "*.js" --glob "*.ts"
Length of output: 572
Script:
#!/bin/bash
# Description: Verify presence of REDIS_PASSWORD, REDIS_HOST, and REDIS_PORT in all .env.example files
# Expected: List of files missing any of the required Redis environment variables
REQUIRED_VARS=("REDIS_PASSWORD" "REDIS_HOST" "REDIS_PORT")
MISSING_VARS=()
for file in $(fd --type f ".env.example" || find . -name ".env.example"); do
for var in "${REQUIRED_VARS[@]}"; do
if ! grep -q "^${var}=" "$file"; then
MISSING_VARS+=("${file}: Missing ${var}")
fi
done
done
if [ ${#MISSING_VARS[@]} -eq 0 ]; then
echo "All .env.example files contain the required Redis environment variables."
else
printf "%s\n" "${MISSING_VARS[@]}"
fi
Length of output: 3030
Script:
#!/bin/bash
# Description: Confirm that the Redis port is standardized across the codebase
# Expected: Ensure that the port is set to 6379 instead of 7381 where applicable
rg "7381" --glob "*.ts" --glob "*.js" --glob "*.yaml" --glob "*.yml" | grep -v "redis-config.ts"
Length of output: 507
| provide: WebhookHttpService, | ||
| useExisting: HttpService, | ||
| }, | ||
| WebhookHttpService, |
There was a problem hiding this comment.
Review module inclusion in providers array.
The simplification of WebhookHttpService provider is good. However, including BullMqModule and OutgoingWebhooksModule in both the imports and providers arrays is unusual and potentially incorrect.
Modules are typically only included in the imports array, not in providers. The providers array is meant for services, not modules.
Consider removing BullMqModule and OutgoingWebhooksModule from the providers array:
providers: [
WebhookHttpService,
AlertService,
AlertRepository,
AlertDefinitionRepository,
WebhookManagerService,
WebhookEventEmitterService,
- BullMqModule,
- OutgoingWebhooksModule,
// TODO: Export to user module
UserService,
UserRepository,
PasswordService,
],Also applies to: 61-62
| if (env.QUEUE_SYSTEM_ENABLED !== true) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Ensure proper type comparison for environment variable QUEUE_SYSTEM_ENABLED.
Environment variables are typically strings. Comparing env.QUEUE_SYSTEM_ENABLED directly to true (a boolean) may not yield the expected result. Consider parsing the environment variable to a boolean before the comparison.
Apply this diff to fix the comparison:
if (env.QUEUE_SYSTEM_ENABLED !== true) {
- return;
+ return;
}Or parse the environment variable:
protected constructor(private queueName: string) {
+ const isQueueSystemEnabled = env.QUEUE_SYSTEM_ENABLED === 'true';
this.connectionOptions = {
...REDIS_CONFIG,
};
- if (env.QUEUE_SYSTEM_ENABLED !== true) {
+ if (!isQueueSystemEnabled) {
return;
}Committable suggestion was skipped due to low confidence.
| protected initializeWorker() { | ||
| this.worker = new Worker( | ||
| this.queueName, | ||
| async (job: Job<T>) => { | ||
| await this.handleJob(job); | ||
| }, | ||
| { connection: this.connectionOptions }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add a check for the queue system being enabled in initializeWorker method.
If the queue system is disabled, initializing the worker is unnecessary and may cause unintended behavior. Add a condition to ensure the worker is only initialized when the queue system is enabled.
Apply this diff to add the check:
protected initializeWorker() {
+ if (!this.queue) {
+ return;
+ }
this.worker = new Worker(
this.queueName,
async (job: Job<T>) => {
await this.handleJob(job);
},
{ connection: this.connectionOptions },
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected initializeWorker() { | |
| this.worker = new Worker( | |
| this.queueName, | |
| async (job: Job<T>) => { | |
| await this.handleJob(job); | |
| }, | |
| { connection: this.connectionOptions }, | |
| ); | |
| } | |
| protected initializeWorker() { | |
| if (!this.queue) { | |
| return; | |
| } | |
| this.worker = new Worker( | |
| this.queueName, | |
| async (job: Job<T>) => { | |
| await this.handleJob(job); | |
| }, | |
| { connection: this.connectionOptions }, | |
| ); | |
| } |
| this.worker?.on('completed', (job: Job) => { | ||
| this.logger.log(`Outgoing Webhook job ${job.id} completed successfully`); | ||
| }); |
There was a problem hiding this comment.
Incorrect log messages: Replace 'Outgoing' with 'Incoming'
The logging statements in the initializeWorker method incorrectly refer to "Outgoing Webhook" jobs, whereas this service handles incoming webhooks. This could lead to confusion when monitoring logs and debugging.
Apply this diff to correct the log messages:
this.worker?.on('completed', (job: Job) => {
- this.logger.log(`Outgoing Webhook job ${job.id} completed successfully`);
+ this.logger.log(`Incoming Webhook job ${job.id} completed successfully`);
});
this.worker?.on('failed', (job, error, prev) => {
- this.logger.error(`Outgoing Webhook job ${job?.id} failed after retries: ${error.message}`);
+ this.logger.error(`Incoming Webhook job ${job?.id} failed after retries: ${error.message}`);
});
this.queue?.on('cleaned', (jobs, type) => {
this.logger.log(
- `${jobs.length} ${type} Outgoing Webhook jobs have been cleaned from the webhook queue`,
+ `${jobs.length} ${type} Incoming Webhook jobs have been cleaned from the webhook queue`,
);
});Also applies to: 29-31, 34-36
| Accept: 'application/json', | ||
| 'Content-Type': 'application/json', | ||
| 'Access-Control-Allow-Origin': '*', | ||
| ...headers, |
There was a problem hiding this comment.
Remove 'Access-Control-Allow-Origin' from request headers
The 'Access-Control-Allow-Origin' header is a response header used by servers to indicate which origins are permitted to read the response. Including it in the request headers has no effect and is unnecessary. It should be removed to prevent confusion and potential misconfigurations.
Apply this diff to remove the unnecessary header:
const signedHeaders = {
Accept: 'application/json',
'Content-Type': 'application/json',
- 'Access-Control-Allow-Origin': '*',
...headers,
...(body && customerConfig?.webhookSharedSecret
? {
'X-HMAC-Signature': sign({
payload: body,
key: customerConfig.webhookSharedSecret,
}),
}
: {}),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| 'Access-Control-Allow-Origin': '*', | |
| ...headers, | |
| Accept: 'application/json', | |
| 'Content-Type': 'application/json', | |
| ...headers, |
| this.logger.error(`Webhook job failed: ${isErrorWithMessage(error) && error.message}`); | ||
| throw error; |
There was a problem hiding this comment.
Improve error logging in the catch block
Using isErrorWithMessage(error) && error.message may result in logging false if isErrorWithMessage(error) returns false. This could lead to unclear error messages in your logs. Consider updating the error logging to handle cases where the error does not have a message property.
Here's a suggested improvement:
this.logger.error(
- `Webhook job failed: ${isErrorWithMessage(error) && error.message}`
+ `Webhook job failed: ${
+ isErrorWithMessage(error) ? error.message : 'An unknown error occurred.'
+ }`
);Alternatively, if it's safe to log the entire error object (ensure no sensitive information is included):
this.logger.error('Webhook job failed:', error);This way, you get a more comprehensive error log, which can aid in debugging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.logger.error(`Webhook job failed: ${isErrorWithMessage(error) && error.message}`); | |
| throw error; | |
| this.logger.error( | |
| `Webhook job failed: ${ | |
| isErrorWithMessage(error) ? error.message : 'An unknown error occurred.' | |
| }` | |
| ); | |
| throw error; |
|
|
||
| const signedHeaders = { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/json', |
There was a problem hiding this comment.
Set 'Content-Type' header appropriately based on body
Currently, the 'Content-Type' header is set to 'application/json' unconditionally. If the body is a string that is not JSON (e.g., plain text or XML), this header may be incorrect. Consider setting the 'Content-Type' header based on the type of body or allow it to be overridden by the headers provided in requestConfig.
Here's how you might adjust the code:
const signedHeaders = {
Accept: 'application/json',
- 'Content-Type': 'application/json',
...headers,
...(body && customerConfig?.webhookSharedSecret
? {
'X-HMAC-Signature': sign({
payload: body,
key: customerConfig.webhookSharedSecret,
}),
}
: {}),
};
+ if (!signedHeaders['Content-Type']) {
+ signedHeaders['Content-Type'] =
+ typeof body === 'string' ? 'text/plain' : 'application/json';
+ }This change ensures that:
- If
'Content-Type'is provided inrequestConfig.headers, it will be used. - Otherwise, it sets
'Content-Type'based on the type ofbody.
Committable suggestion was skipped due to low confidence.
|
|
||
| @common.Injectable() | ||
| export abstract class WebhookHttpService extends HttpService {} | ||
| export class WebhookHttpService {} |
There was a problem hiding this comment.
Unused Empty Class WebhookHttpService
The class WebhookHttpService is currently empty without any methods or properties. If it's no longer needed, consider removing it to keep the codebase clean. If it's a placeholder for future development, adding a comment to indicate its intended use would be helpful.
| private async handleRetryStrategy(status: number, job: Job<WebhookJobData>) { | ||
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | ||
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | ||
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | ||
| } | ||
|
|
||
| let delayMs: number; | ||
|
|
||
| switch (status) { | ||
| case HttpStatusCode.TooManyRequests: | ||
| case HttpStatusCode.InternalServerError: | ||
| case HttpStatusCode.BadGateway: | ||
| case HttpStatusCode.ServiceUnavailable: | ||
| case HttpStatusCode.GatewayTimeout: | ||
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | ||
| break; | ||
|
|
||
| case HttpStatusCode.RequestTimeout: | ||
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | ||
| break; | ||
|
|
||
| case HttpStatusCode.BadRequest: | ||
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | ||
|
|
||
| default: | ||
| throw new Error(`Webhook job failed with status ${status}: Unexpected Error`); | ||
| } | ||
|
|
||
| await this.retryJob(job, delayMs); | ||
| } |
There was a problem hiding this comment.
Fix handling of successful response statuses in handleRetryStrategy
Currently, the handleRetryStrategy method throws an error for any status code not explicitly handled in the switch statement, including successful status codes like 200 (OK). This means that even when the webhook invocation is successful, the job will be marked as failed.
To resolve this, adjust the method to handle successful statuses appropriately. Here's the suggested change:
+ case HttpStatusCode.Ok:
+ case HttpStatusCode.Created:
+ case HttpStatusCode.Accepted:
+ // Success statuses, no action needed
+ break;
default:
- throw new Error(`Webhook job failed with status ${status}: Unexpected Error`);
+ delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff for unexpected errors
+ await this.retryJob(job, delayMs);
+ break;This change ensures that successful responses do not cause the job to fail, and unexpected errors are retried with exponential backoff.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async handleRetryStrategy(status: number, job: Job<WebhookJobData>) { | |
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | |
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | |
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | |
| } | |
| let delayMs: number; | |
| switch (status) { | |
| case HttpStatusCode.TooManyRequests: | |
| case HttpStatusCode.InternalServerError: | |
| case HttpStatusCode.BadGateway: | |
| case HttpStatusCode.ServiceUnavailable: | |
| case HttpStatusCode.GatewayTimeout: | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | |
| break; | |
| case HttpStatusCode.RequestTimeout: | |
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | |
| break; | |
| case HttpStatusCode.BadRequest: | |
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | |
| default: | |
| throw new Error(`Webhook job failed with status ${status}: Unexpected Error`); | |
| } | |
| await this.retryJob(job, delayMs); | |
| } | |
| private async handleRetryStrategy(status: number, job: Job<WebhookJobData>) { | |
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | |
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | |
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | |
| } | |
| let delayMs: number; | |
| switch (status) { | |
| case HttpStatusCode.TooManyRequests: | |
| case HttpStatusCode.InternalServerError: | |
| case HttpStatusCode.BadGateway: | |
| case HttpStatusCode.ServiceUnavailable: | |
| case HttpStatusCode.GatewayTimeout: | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | |
| break; | |
| case HttpStatusCode.RequestTimeout: | |
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | |
| break; | |
| case HttpStatusCode.BadRequest: | |
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | |
| case HttpStatusCode.Ok: | |
| case HttpStatusCode.Created: | |
| case HttpStatusCode.Accepted: | |
| // Success statuses, no action needed | |
| break; | |
| default: | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff for unexpected errors | |
| await this.retryJob(job, delayMs); | |
| break; | |
| } | |
| await this.retryJob(job, delayMs); | |
| } |
# Conflicts: # services/workflows-service/package.json # services/workflows-service/prisma/data-migrations
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
services/workflows-service/.env.example (1)
41-41: Consider adding a USE_REDIS variable.The
IS_WORKER_SERVICE=falseflag is a good addition for determining the service type. However, as suggested in a previous comment, consider also adding aUSE_REDISvariable to toggle between Redis and in-memory implementations.Consider adding the following line:
USE_REDIS=trueThis would allow for easier switching between Redis and in-memory implementations in different environments.
services/workflows-service/src/main.ts (2)
54-60: LGTM: Conditional creation of WorkerAppModule.The addition of conditional logic to create either WorkerAppModule or AppModule based on the IS_WORKER_SERVICE environment variable is a good approach for supporting different application modes. This allows for flexibility in deployment and testing.
However, consider adding an explicit else statement for clarity:
if (env.IS_WORKER_SERVICE) { return await NestFactory.create(WorkerAppModule, { bufferLogs: true, snapshot: true, }); } else { const app = await NestFactory.create(AppModule, { // ... existing options }); // ... rest of the setup }This makes the flow more explicit and easier to follow.
Worker service includes Sentry integration but lacks other middleware configurations.
services/workflows-service/src/worker-app.module.tsimports and usesSentryModule.- No usage of
helmetor other middleware configurations found.Consider verifying if the worker service requires additional security configurations or middleware similar to the main application. Implement them if necessary to maintain consistency and enhance security.
🔗 Analysis chain
Line range hint
61-186: Verify worker service requirements.The current implementation bypasses all middleware and configuration setups for the worker service. While this may be intentional, it's important to verify that the worker service doesn't require any of these configurations.
Consider the following:
- Does the worker service need any security configurations (e.g., helmet)?
- Is error handling (e.g., Sentry) required for the worker service?
- Are there any shared configurations that both the main app and worker service should use?
If any of these are needed, consider extracting shared configurations into a separate function that can be called for both the main app and the worker service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of middleware or configurations in the worker module rg -t typescript 'middleware|configur|helmet|sentry' src/worker-app.module.tsLength of output: 114
Script:
#!/bin/bash # Search for any usage of middleware or configurations in the worker module with corrected file type rg -t ts 'middleware|configur|helmet|sentry' src/worker-app.module.tsLength of output: 136
Script:
#!/bin/bash # Search for TypeScript module files related to the worker service fd -e ts -e tsx -i 'worker.*module.ts'Length of output: 92
Script:
#!/bin/bash # Search for middleware or configuration usages in the worker module rg -i 'middleware|configur|helmet|sentry' services/workflows-service/src/worker-app.module.tsLength of output: 168
Script:
#!/bin/bash # Verify the existence of the worker module file and search for middleware or configuration usages if [ -f services/workflows-service/src/worker-app.module.ts ]; then rg -i 'middleware|configur|helmet|sentry' services/workflows-service/src/worker-app.module.ts else echo "File services/workflows-service/src/worker-app.module.ts does not exist." fiLength of output: 233
services/workflows-service/src/worker-app.module.ts (1)
2-13: Inconsistent import paths: Standardize the import statementsThe import statements use both absolute paths (using
@/) and relative paths (using./). For consistency and maintainability, consider standardizing the import paths throughout the file.Suggested Change:
Choose one import style and apply it consistently. For example, if using absolute paths with module aliases:
- Update relative imports to use absolute paths with the appropriate alias.
Apply this diff to adjust the relative imports to use absolute paths:
-import { HealthModule } from './health/health.module'; -import { PrismaModule } from './prisma/prisma.module'; -import { hashKey } from './customer/api-key/utils'; +import { HealthModule } from '@/health/health.module'; +import { PrismaModule } from '@/prisma/prisma.module'; +import { hashKey } from '@/customer/api-key/utils';Ensure that your project's module resolution is configured to recognize the
@alias (e.g., in thetsconfig.jsonfile).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- services/workflows-service/.env.example (1 hunks)
- services/workflows-service/prisma/data-migrations (1 hunks)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
- services/workflows-service/src/main.ts (2 hunks)
- services/workflows-service/src/worker-app.module.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts
🧰 Additional context used
🔇 Additional comments (8)
services/workflows-service/.env.example (2)
38-38: LGTM: Redis host configuration looks good.The
REDIS_HOST=localhostsetting is appropriate for local development environments.
39-39: Verify the non-standard Redis port.The
REDIS_PORT=7381uses a non-standard port for Redis. While this may be intentional to avoid conflicts, please ensure this port is consistent with your Docker configuration and other services that might interact with Redis.✅ Verification successful
To further verify the Redis port configuration, please run the following script:
Redis port configuration is consistent and verified.
The
REDIS_PORT=7381is intentionally set to avoid conflicts and is consistently mapped in the Docker configuration:
- Docker Mapping: Host port
7381➔ Container port6379.- Application Configuration: Uses
REDIS_PORTfrom the environment with a default of7381.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the Redis port is consistent in the Docker configuration. # Test: Search for the Redis port in Docker Compose files echo "Searching for Redis port in Docker Compose files:" rg --type yaml 'redis.*port.*7381' services/workflows-service/Length of output: 173
Script:
#!/bin/bash # Description: Search for all occurrences of REDIS_PORT to ensure consistency across the codebase. echo "Searching for REDIS_PORT in the codebase:" rg 'REDIS_PORT' .Length of output: 531
services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (4)
1-5: LGTM: Import statements are well-organized and complete.The import statements are logically ordered and include all necessary dependencies for the class functionality.
29-29: LGTM: Abstract method declaration is correct.The
handleJobabstract method is properly declared, ensuring that subclasses will implement the specific job handling logic.
59-61: LGTM:onModuleDestroymethod is well-implemented.The
onModuleDestroymethod correctly handles resource cleanup:
- It uses
Promise.allto close both worker and queue concurrently, which is efficient.- Optional chaining is used to safely handle cases where worker or queue might be undefined.
This implementation ensures proper resource management when the module is destroyed.
1-62: Overall assessment: Well-structured base class with minor improvements needed.The
BaseQueueWorkerServiceclass provides a solid foundation for implementing job queue management in a NestJS application using BullMQ. It follows best practices for both NestJS (using@Injectable()and implementingOnModuleDestroy) and BullMQ (proper queue and worker setup with event handling).Key strengths:
- Abstract design allowing for flexible job handling in subclasses.
- Proper resource management with
onModuleDestroy.- Comprehensive event logging for job statuses.
Areas for improvement:
- Environment variable parsing for boolean values.
- Error handling in the
addJobmethod when the queue system is disabled.- Additional check in
initializeWorkerto prevent initialization when the queue system is disabled.Addressing these minor issues will further enhance the robustness and reliability of this base class. Overall, this is a well-implemented foundation for queue worker services.
services/workflows-service/src/main.ts (2)
21-21: LGTM: New import for WorkerAppModule.The import statement for WorkerAppModule is correctly added. This new module likely contains the worker-specific functionality for the application.
Line range hint
1-186: Summary of changes and suggestions.The changes to
main.tssuccessfully introduce a new worker service mode, aligning with the PR objectives of enhancing Redis integration and improving application flexibility. The implementation allows for easy switching between regular and worker service modes using an environment variable.Key points and suggestions:
- The import and conditional creation of WorkerAppModule are well-implemented.
- Consider adding an explicit else statement for clarity in the main function.
- Verify the requirements for the worker service, especially regarding middleware and configurations.
Overall, these changes provide a solid foundation for integrating the worker service functionality. Ensure thorough testing of both modes to guarantee proper functionality in different environments.
| @@ -35,3 +35,7 @@ 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.
Security concern: Weak Redis password.
The REDIS_PASSWORD=password is a significant security risk. Even though this is an example file, it's crucial to:
- Use a strong, unique password for each environment.
- Never commit actual passwords to version control.
- Consider using a secret management system for production environments.
Replace the current line with:
-REDIS_PASSWORD=password
+REDIS_PASSWORD=<strong-unique-password>Also, add a comment above this line:
# IMPORTANT: Replace with a strong, unique password. Never commit the actual password.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| REDIS_PASSWORD=password | |
| # IMPORTANT: Replace with a strong, unique password. Never commit the actual password. | |
| REDIS_PASSWORD=<strong-unique-password> |
| @Injectable() | ||
| export abstract class BaseQueueWorkerService<T = any> implements OnModuleDestroy { | ||
| protected queue?: Queue; | ||
| protected worker?: Worker; | ||
| protected connectionOptions: ConnectionOptions; | ||
|
|
||
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | ||
| this.connectionOptions = { | ||
| ...REDIS_CONFIG, | ||
| }; | ||
|
|
||
| if (env.QUEUE_SYSTEM_ENABLED !== true) { | ||
| return; | ||
| } | ||
|
|
||
| this.queue = new Queue(queueName, { connection: this.connectionOptions }); | ||
|
|
||
| if (env.IS_WORKER_SERVICE !== true) { | ||
| this.initializeWorker(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve environment variable comparison in the constructor.
The class structure and decorator usage are appropriate. However, there's a potential issue with the environment variable comparison:
- Environment variables are typically strings, so comparing
env.QUEUE_SYSTEM_ENABLEDdirectly totrue(a boolean) may not yield the expected result. - The same issue applies to the
env.IS_WORKER_SERVICEcomparison.
Consider parsing the environment variables to booleans before comparison. Apply this diff to fix the comparisons:
- if (env.QUEUE_SYSTEM_ENABLED !== true) {
+ if (env.QUEUE_SYSTEM_ENABLED !== 'true') {
return;
}
this.queue = new Queue(queueName, { connection: this.connectionOptions });
- if (env.IS_WORKER_SERVICE !== true) {
+ if (env.IS_WORKER_SERVICE !== 'true') {
this.initializeWorker();
}Alternatively, you could use a helper function to parse boolean environment variables consistently throughout the application.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Injectable() | |
| export abstract class BaseQueueWorkerService<T = any> implements OnModuleDestroy { | |
| protected queue?: Queue; | |
| protected worker?: Worker; | |
| protected connectionOptions: ConnectionOptions; | |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (env.QUEUE_SYSTEM_ENABLED !== true) { | |
| return; | |
| } | |
| this.queue = new Queue(queueName, { connection: this.connectionOptions }); | |
| if (env.IS_WORKER_SERVICE !== true) { | |
| this.initializeWorker(); | |
| } | |
| } | |
| @Injectable() | |
| export abstract class BaseQueueWorkerService<T = any> implements OnModuleDestroy { | |
| protected queue?: Queue; | |
| protected worker?: Worker; | |
| protected connectionOptions: ConnectionOptions; | |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (env.QUEUE_SYSTEM_ENABLED !== 'true') { | |
| return; | |
| } | |
| this.queue = new Queue(queueName, { connection: this.connectionOptions }); | |
| if (env.IS_WORKER_SERVICE !== 'true') { | |
| this.initializeWorker(); | |
| } | |
| } |
| @@ -90,6 +90,17 @@ 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.
🛠️ Refactor suggestion
Enhance the REDIS_HOST configuration
While adding REDIS_HOST is a good start, consider improving it based on previous suggestions:
- Add a description for better maintainability.
- Consider adding validation to ensure it's a valid hostname or IP address.
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.
| @@ -90,6 +90,17 @@ | |||
| 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(), | |||
| REDIS_PASSWORD: z.string(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make REDIS_PASSWORD optional and add a description
The current implementation makes REDIS_PASSWORD mandatory, which might not be suitable for all Redis configurations. Consider the following improvements:
- Make the password optional.
- Add a description for better maintainability.
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.
| @@ -90,6 +90,17 @@ | |||
| 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(), | |||
| REDIS_PASSWORD: z.string(), | |||
| REDIS_PORT: z.string().transform(value => Number(value)), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance REDIS_PORT configuration with validation
The current implementation transforms the port to a number but lacks validation. Consider adding the following improvements:
- Add a description for better maintainability.
- Validate that the port is within a valid range (e.g., 1024-65535).
- Provide a default value if needed.
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).
| QUEUE_SYSTEM_ENABLED: z | ||
| .string() | ||
| .transform(value => Boolean(value)) | ||
| .default('false'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance QUEUE_SYSTEM_ENABLED with description and explicit typing
The implementation of QUEUE_SYSTEM_ENABLED is good, but it can be improved:
- Add a description for better maintainability.
- Use more explicit typing for better clarity.
Here's a suggested implementation:
QUEUE_SYSTEM_ENABLED: z
.enum(['true', 'false'])
.default('false')
.transform(value => value === 'true')
.describe('Enable or disable the queue system'),This implementation adds a description, uses an enum for explicit typing, and simplifies the boolean transformation.
| IS_WORKER_SERVICE: z | ||
| .string() | ||
| .transform(value => Boolean(value)) | ||
| .default('false'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance IS_WORKER_SERVICE configuration
The IS_WORKER_SERVICE configuration can be improved similarly to QUEUE_SYSTEM_ENABLED:
- Add a description for better maintainability.
- Use more explicit typing for better clarity.
Here's a suggested implementation:
IS_WORKER_SERVICE: z
.enum(['true', 'false'])
.default('false')
.transform(value => value === 'true')
.describe('Indicates whether this instance is a worker service'),This implementation adds a description, uses an enum for explicit typing, and simplifies the boolean transformation, maintaining consistency with the QUEUE_SYSTEM_ENABLED configuration.
| if (!result.success) { | ||
| const errors = result.error.errors.map(zodIssue => ({ | ||
| message: `❌ ${zodIssue.message}`, | ||
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array |
There was a problem hiding this comment.
Typo in comment: 'excepts' should be 'expects'
There's a typographical error in the comment. The word "excepts" should be "expects" to accurately convey the intended meaning.
Apply this diff to correct the typo:
- path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array
+ path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message expects array📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array | |
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message expects array |
| export const validate = async (config: Record<string, unknown>) => { | ||
| const zodEnvSchema = z | ||
| .object(serverEnvSchema) | ||
| .refine(data => data.HASHING_KEY_SECRET || data.HASHING_KEY_SECRET_BASE64, { | ||
| message: 'At least one of HASHING_KEY_SECRET or HASHING_KEY_SECRET_BASE64 should be present', | ||
| path: ['HASHING_KEY_SECRET', 'HASHING_KEY_SECRET_BASE64'], | ||
| }); | ||
|
|
||
| const result = zodEnvSchema.safeParse(config); | ||
|
|
||
| if (!result.success) { | ||
| const errors = result.error.errors.map(zodIssue => ({ | ||
| message: `❌ ${zodIssue.message}`, | ||
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array | ||
| })); | ||
|
|
||
| throw new Error(JSON.stringify(errors, null, 2)); | ||
| } | ||
|
|
||
| // validate salt value | ||
| await hashKey('check salt value'); | ||
|
|
||
| return result.data; | ||
| }; |
There was a problem hiding this comment.
Potential Issue: validate function should be synchronous with ConfigModule
The validate function passed to ConfigModule.forRoot() is defined as an async function. However, NestJS ConfigModule expects a synchronous function for validation. Using an async function may lead to unexpected behavior or errors during the configuration process since the configuration loading is synchronous.
Suggested Change:
Refactor the validate function to be synchronous. If hashKey is an asynchronous operation, consider performing the salt validation elsewhere in the application where asynchronous operations are allowed.
Apply this diff to make the validate function synchronous:
-export const validate = async (config: Record<string, unknown>) => {
+export const validate = (config: Record<string, unknown>) => {
const zodEnvSchema = z
.object(serverEnvSchema)
.refine(data => data.HASHING_KEY_SECRET || data.HASHING_KEY_SECRET_BASE64, {
message: 'At least one of HASHING_KEY_SECRET or HASHING_KEY_SECRET_BASE64 should be present',
path: ['HASHING_KEY_SECRET', 'HASHING_KEY_SECRET_BASE64'],
});
const result = zodEnvSchema.safeParse(config);
if (!result.success) {
const errors = result.error.errors.map(zodIssue => ({
message: `❌ ${zodIssue.message}`,
path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message expects array
}));
throw new Error(JSON.stringify(errors, null, 2));
}
- // validate salt value
- await hashKey('check salt value');
return result.data;
};Then, perform the salt value validation after the application has initialized, possibly in the onModuleInit lifecycle hook of a suitable module.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const validate = async (config: Record<string, unknown>) => { | |
| const zodEnvSchema = z | |
| .object(serverEnvSchema) | |
| .refine(data => data.HASHING_KEY_SECRET || data.HASHING_KEY_SECRET_BASE64, { | |
| message: 'At least one of HASHING_KEY_SECRET or HASHING_KEY_SECRET_BASE64 should be present', | |
| path: ['HASHING_KEY_SECRET', 'HASHING_KEY_SECRET_BASE64'], | |
| }); | |
| const result = zodEnvSchema.safeParse(config); | |
| if (!result.success) { | |
| const errors = result.error.errors.map(zodIssue => ({ | |
| message: `❌ ${zodIssue.message}`, | |
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array | |
| })); | |
| throw new Error(JSON.stringify(errors, null, 2)); | |
| } | |
| // validate salt value | |
| await hashKey('check salt value'); | |
| return result.data; | |
| }; | |
| export const validate = (config: Record<string, unknown>) => { | |
| const zodEnvSchema = z | |
| .object(serverEnvSchema) | |
| .refine(data => data.HASHING_KEY_SECRET || data.HASHING_KEY_SECRET_BASE64, { | |
| message: 'At least one of HASHING_KEY_SECRET or HASHING_KEY_SECRET_BASE64 should be present', | |
| path: ['HASHING_KEY_SECRET', 'HASHING_KEY_SECRET_BASE64'], | |
| }); | |
| const result = zodEnvSchema.safeParse(config); | |
| if (!result.success) { | |
| const errors = result.error.errors.map(zodIssue => ({ | |
| message: `❌ ${zodIssue.message}`, | |
| path: zodIssue.path.join('.'), // Backwards compatibility - Legacy code message excepts array | |
| })); | |
| throw new Error(JSON.stringify(errors, null, 2)); | |
| } | |
| return result.data; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
services/workflows-service/src/worker-main.ts (1)
6-19: LGTM: Well-structured worker setup with a minor suggestion.The
workerMainfunction follows NestJS best practices for setting up a worker application. It correctly initializes the application, sets up logging, and applies middleware for context management.Consider adding error handling to catch and log any initialization errors:
const workerMain = async () => { - const app = await NestFactory.create(WorkerAppModule, { - bufferLogs: true, - snapshot: true, - }); + try { + const app = await NestFactory.create(WorkerAppModule, { + bufferLogs: true, + snapshot: true, + }); - const logger = app.get(AppLoggerService); + const logger = app.get(AppLoggerService); - app.useLogger(logger); - app.use(new ClsMiddleware({}).use); - logger.log('Worker started'); + app.useLogger(logger); + app.use(new ClsMiddleware({}).use); + logger.log('Worker started'); - return app; + return app; + } catch (error) { + console.error('Failed to initialize worker:', error); + throw error; + } };This change will ensure that any initialization errors are properly logged and propagated.
services/workflows-service/package.json (3)
14-14: LGTM: New worker script added.The addition of a separate worker script is a good practice for handling background jobs. This separation of concerns can improve the application's scalability and maintainability.
Consider adding a comment or updating the documentation to explain the purpose and usage of this new worker script.
35-36: LGTM: Redis management scripts added.The addition of scripts for managing the Redis container is a good practice. It allows for easy setup and teardown of the Redis service during development and testing.
Consider adding a
docker:redis:logsscript to easily view Redis logs, which can be helpful for debugging:"docker:redis:logs": "docker compose -f docker-compose.redis.yml logs -f"
57-59: LGTM: Bull Board dependencies added for queue monitoring.The addition of Bull Board dependencies (@bull-board/api, @bull-board/express, @bull-board/nestjs) is a good choice for monitoring BullMQ queues. This will provide valuable insights into job processing and queue status.
Consider adding documentation on how to access and use the Bull Board UI in your project's README or developer documentation.
services/workflows-service/src/events/workflow-completed-webhook-caller.ts (1)
113-126: Remove unnecessary 'await' keyword before returnUsing
return awaitin an async function is redundant because it adds an extra microtask without any benefit. Consider removing theawaitkeyword to improve performance.Apply this diff to simplify the return statement:
- return await this.outgoingWebhookQueueService.addJob({ + return this.outgoingWebhookQueueService.addJob({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
- services/workflows-service/src/events/document-changed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/events/workflow-completed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/worker-app.module.ts (1 hunks)
- services/workflows-service/src/worker-main.ts (1 hunks)
- services/workflows-service/src/workflow/workflow.module.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts
- services/workflows-service/src/env.ts
- services/workflows-service/src/worker-app.module.ts
🧰 Additional context used
🔇 Additional comments (16)
services/workflows-service/src/worker-main.ts (1)
1-4: LGTM: Imports are appropriate for the worker functionality.The imports cover the necessary modules for creating a NestJS worker application, including logging, context management, and the main application module.
services/workflows-service/src/workflow/workflow.module.ts (3)
50-50: LGTM: BullMqModule import added correctly.The import statement for BullMqModule is properly formatted and uses the correct path aliasing. This addition aligns with the PR objective of integrating BullMQ for Redis-based job queue management.
69-69: LGTM: BullMqModule correctly added to imports.The BullMqModule has been properly added to the imports array of the WorkflowModule. This addition is necessary for integrating BullMQ functionality into the Workflow service and aligns with the PR objectives.
Line range hint
50-69: Verify BullMqModule configuration and usage.The integration of BullMqModule into the WorkflowModule aligns well with the PR objectives. However, to ensure a complete implementation:
- Verify that BullMqModule is properly configured, possibly in a separate configuration file.
- Check if there are any other files in the Workflow service that need to be updated to use BullMQ functionality.
- Consider adding unit tests to verify the integration of BullMqModule.
To help with verification, you can run the following script:
This script will help identify if BullMqModule is properly configured and used within the Workflow service, and if there are any new unit tests related to the BullMQ integration.
services/workflows-service/package.json (3)
8-8: LGTM: Setup script updated to include Redis management.The setup script has been appropriately modified to include Redis management. It now includes both
docker:redis:downanddocker:redis, addressing the previous comment by MatanYadaev. The order of operations is logical, ensuring a clean setup environment.
16-16: LGTM: Production worker script added.The new "worker:prod" script appropriately runs database migrations before starting the worker application from the compiled source. This ensures that the database schema is up-to-date in the production environment before the worker starts processing jobs.
62-62: LGTM: BullMQ dependencies added for job queue functionality.The addition of @nestjs/bullmq and bullmq dependencies is appropriate for implementing job queue functionality in the project. This addresses the need for robust background job processing.
Regarding past comments:
- The Redis client is typically included with BullMQ, so it doesn't need to be added separately.
- Bull Board (added earlier) is a more modern alternative to bull-arena, effectively addressing the suggestion for a UI component.
Also applies to: 89-89
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (2)
13-14: Approved: Necessary imports addedThe imports for
envandOutgoingWebhookQueueServiceare correctly added and necessary for the new functionality.
26-26: Approved: Injected 'OutgoingWebhookQueueService' into the constructorThe
OutgoingWebhookQueueServiceis properly injected into the constructor to enable queueing of webhook requests.services/workflows-service/src/events/workflow-completed-webhook-caller.ts (3)
113-126:⚠️ Potential issueEnsure webhook headers are included when queuing requests
In the queue-enabled scenario, the
requestConfighas an emptyheadersobject. In the direct webhook call, headers likeX-AuthorizationandX-HMAC-Signatureare included. Verify that these headers are added when the webhook is processed from the queue to maintain consistent authentication.Run the following script to check if headers are set in the queued webhook processing:
#!/bin/bash # Description: Search for header assignment using 'webhookSharedSecret' in queue processing code fd -e ts 'outgoing-webhook' | xargs rg 'headers.*webhookSharedSecret'
29-29: Ensure 'OutgoingWebhookQueueService' is provided in the module's providers arrayThe
OutgoingWebhookQueueServiceis injected into the constructor. Make sure it is included in the module'sprovidersarray to enable proper dependency injection.Run the following script to check if
OutgoingWebhookQueueServiceis registered in the module:#!/bin/bash # Description: Search for module files that provide 'OutgoingWebhookQueueService' fd -e ts 'module.ts' | xargs rg 'providers\s*:\s*\[.*OutgoingWebhookQueueService.*\]'
15-16: Verify imports for 'env' and 'OutgoingWebhookQueueService' are correctly resolvedThe imports for
envandOutgoingWebhookQueueServicehave been added. Ensure that these modules are properly exported and there are no issues with module resolution.Run the following script to confirm that the imports are valid:
services/workflows-service/src/events/document-changed-webhook-caller.ts (4)
16-17: LGTM!The new imports are correctly added and paths are accurate.
39-39: LGTM!Dependency injection of
OutgoingWebhookQueueServicein the constructor is properly implemented.
178-201: LGTM!The payload for the webhook is constructed correctly with all necessary fields included.
202-215: Ensure headers are correctly included in queued webhook requestsWhen
env.QUEUE_SYSTEM_ENABLEDistrue, the webhook request is enqueued with empty headers inrequestConfig. In the direct HTTP POST request, headers includeX-AuthorizationandX-HMAC-Signature. Please verify that these headers are added when the queued job is processed to ensure proper authentication and signature verification.Run the following script to verify that the
OutgoingWebhookQueueServiceor its associated job processor includes the necessary headers:
| return app; | ||
| }; | ||
|
|
||
| module.exports = workerMain(); |
There was a problem hiding this comment.
Consider exporting the function instead of its execution result.
The current export might cause issues if the module is imported synchronously, as it immediately executes the async function. This approach also doesn't allow for error handling or customization by the importing module.
Consider changing the export to:
-module.exports = workerMain();
+module.exports = workerMain;This change allows the importing module to control when and how the worker is initialized, and provides better error handling capabilities.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module.exports = workerMain(); | |
| module.exports = workerMain; |
| @@ -89,6 +92,21 @@ export class WorkflowStateChangedWebhookCaller { | |||
| data: data.runtimeData.context, | |||
| }; | |||
|
|
|||
| if (env.QUEUE_SYSTEM_ENABLED) { | |||
There was a problem hiding this comment.
Ensure 'QUEUE_SYSTEM_ENABLED' is explicitly compared to 'true'
Environment variables are strings, so the condition if (env.QUEUE_SYSTEM_ENABLED) may not work as intended. If env.QUEUE_SYSTEM_ENABLED is set to 'false' or any non-empty string, the condition will evaluate as truthy. Consider explicitly comparing the value to 'true'.
Apply this diff to fix the condition:
-if (env.QUEUE_SYSTEM_ENABLED) {
+if (env.QUEUE_SYSTEM_ENABLED === 'true') {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (env.QUEUE_SYSTEM_ENABLED) { | |
| if (env.QUEUE_SYSTEM_ENABLED === 'true') { |
| if (env.QUEUE_SYSTEM_ENABLED) { | ||
| return await this.outgoingWebhookQueueService.addJob({ | ||
| requestConfig: { | ||
| url, | ||
| method: 'POST', | ||
| headers: {}, | ||
| body: payload, | ||
| timeout: 15_000, | ||
| }, | ||
| customerConfig: { | ||
| webhookSharedSecret, | ||
| }, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle potential errors from 'addJob' to prevent unhandled exceptions
The addJob method may throw an error, and currently, it's not wrapped in a try-catch block. This could lead to unhandled exceptions if an error occurs while adding the job to the queue. Consider wrapping the addJob call in a try-catch block to handle any potential errors and maintain consistency with error handling in the rest of the method.
Apply this diff to wrap the addJob call in a try-catch block:
if (env.QUEUE_SYSTEM_ENABLED === 'true') {
+ try {
await this.outgoingWebhookQueueService.addJob({
requestConfig: {
url,
method: 'POST',
headers: {},
body: payload,
timeout: 15_000,
},
customerConfig: {
webhookSharedSecret,
},
});
+ return;
+ } catch (error) {
+ this.logger.error('Failed to add job to queue', { error });
+ alertWebhookFailure(error);
+ }
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (env.QUEUE_SYSTEM_ENABLED) { | |
| return await this.outgoingWebhookQueueService.addJob({ | |
| requestConfig: { | |
| url, | |
| method: 'POST', | |
| headers: {}, | |
| body: payload, | |
| timeout: 15_000, | |
| }, | |
| customerConfig: { | |
| webhookSharedSecret, | |
| }, | |
| }); | |
| } | |
| if (env.QUEUE_SYSTEM_ENABLED === 'true') { | |
| try { | |
| await this.outgoingWebhookQueueService.addJob({ | |
| requestConfig: { | |
| url, | |
| method: 'POST', | |
| headers: {}, | |
| body: payload, | |
| timeout: 15_000, | |
| }, | |
| customerConfig: { | |
| webhookSharedSecret, | |
| }, | |
| }); | |
| return; | |
| } catch (error) { | |
| this.logger.error('Failed to add job to queue', { error }); | |
| alertWebhookFailure(error); | |
| } | |
| return; | |
| } |
| if (env.QUEUE_SYSTEM_ENABLED) { | ||
| return await this.outgoingWebhookQueueService.addJob({ | ||
| requestConfig: { | ||
| url, | ||
| method: 'POST', | ||
| headers: {}, | ||
| body: payload, | ||
| timeout: 15_000, | ||
| }, | ||
| customerConfig: { | ||
| webhookSharedSecret, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Correct the evaluation of 'QUEUE_SYSTEM_ENABLED' environment variable
Environment variables are strings, so env.QUEUE_SYSTEM_ENABLED may not behave as expected in a boolean context. If the variable is set to 'false', it will still be truthy. Explicitly compare the value to ensure correct behavior.
Apply this diff to properly evaluate the environment variable:
- if (env.QUEUE_SYSTEM_ENABLED) {
+ if (env.QUEUE_SYSTEM_ENABLED === 'true') {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (env.QUEUE_SYSTEM_ENABLED) { | |
| return await this.outgoingWebhookQueueService.addJob({ | |
| requestConfig: { | |
| url, | |
| method: 'POST', | |
| headers: {}, | |
| body: payload, | |
| timeout: 15_000, | |
| }, | |
| customerConfig: { | |
| webhookSharedSecret, | |
| }, | |
| }); | |
| } | |
| if (env.QUEUE_SYSTEM_ENABLED === 'true') { | |
| return await this.outgoingWebhookQueueService.addJob({ | |
| requestConfig: { | |
| url, | |
| method: 'POST', | |
| headers: {}, | |
| body: payload, | |
| timeout: 15_000, | |
| }, | |
| customerConfig: { | |
| webhookSharedSecret, | |
| }, | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (1)
109-111: Consider adding logging after adding the job to the queueTo maintain consistency and aid in debugging, consider logging a message after successfully adding a job to the queue. This mirrors the logging behavior in the direct webhook invocation.
You could implement the logging as follows:
if (env.QUEUE_SYSTEM_ENABLED) { const job = await this.outgoingWebhookQueueService.addJob(webhookArgs); this.logger.log('Job added to queue', { jobId: job.id }); return; }services/workflows-service/src/events/document-changed-webhook-caller.ts (2)
16-16: Consistent Environment Variable ManagementThe import
import { env } from '@/env';introducesenvfor environment variables. Ensure that theenvobject is populated correctly and consistently throughout the application. Inconsistent usage might lead to environment variables not being read as expected.Consider centralizing environment variable access to maintain consistency.
Line range hint
221-234: Enhance Error Logging for Webhook InvocationIn the
catchblock, the error is logged, but without detailed error information. Providing more context can aid in debugging.Consider logging additional error details:
this.logger.error('Failed to send webhook', { id, message: error?.message, stack: error?.stack, responseData: error?.response?.data, });This includes error stack traces and any response data from failed HTTP requests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1 hunks)
- services/workflows-service/src/events/document-changed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/events/workflow-completed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (3 hunks)
- services/workflows-service/src/workflow/workflow.module.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/workflow/workflow.module.ts
🧰 Additional context used
🔇 Additional comments (11)
services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (2)
1-9: LGTM: Imports and decorator are appropriate.The imports cover all necessary dependencies for BullMQ, NestJS, and custom configurations. The
@Injectable()decorator is correctly used to make the class available for dependency injection in NestJS.
66-68: LGTM: Proper resource cleanup inonModuleDestroy.The
onModuleDestroymethod correctly implements theOnModuleDestroyinterface, ensuring proper cleanup of both the worker and queue resources. The use ofPromise.allis an efficient way to handle multiple asynchronous operations.services/workflows-service/src/events/workflow-completed-webhook-caller.ts (3)
12-12: LGTM: New dependencies and constructor parameters.The addition of new imports and constructor parameters for
OutgoingWebhookQueueServiceandOutgoingWebhooksServiceis consistent with the changes in thesendWebhookmethod. This approach follows good dependency injection principles, enhancing modularity and testability.Also applies to: 15-17, 30-31
86-126: LGTM: Improved webhook payload and flexible sending mechanism.The changes to the
sendWebhookmethod are well-structured:
- The new payload object is more comprehensive and organized.
- The
webhookArgsobject encapsulates request and customer configurations cleanly.- The introduction of a queue system option allows for better handling of high loads or network issues.
- Error handling and logging are preserved, maintaining robustness.
These changes enhance the flexibility and reliability of the webhook sending process.
Line range hint
132-138: LGTM: Improved webhook invocation using a dedicated service.The replacement of the direct Axios call with
outgoingWebhooksService.invokeWebhookis a positive change:
- It promotes better separation of concerns by delegating the webhook invocation to a dedicated service.
- The preservation of result logging maintains good observability.
This change enhances the modularity and maintainability of the code.
services/workflows-service/src/events/workflow-state-changed-webhook-caller.ts (3)
12-14: Imports are correctly addedThe new imports for
env,OutgoingWebhookQueueService, andOutgoingWebhooksServiceare appropriate and necessary for the new functionality introduced.
26-27: Constructor parameters are appropriately updatedIncluding
outgoingWebhookQueueServiceandoutgoingWebhooksServicein the constructor aligns with the required dependencies for handling outgoing webhooks.
96-107: 'webhookArgs' object is constructed correctlyThe
webhookArgsobject encapsulates the necessary request and customer configurations, and usingas constensures the object is read-only, which is appropriate for preventing unintended mutations.services/workflows-service/src/events/document-changed-webhook-caller.ts (3)
17-18: Ensure New Services Are Registered in ModuleThe
OutgoingWebhookQueueServiceandOutgoingWebhooksServiceare newly imported services. Please verify that these services are properly registered in the module'sprovidersarray to ensure they are correctly injected, preventing any runtimeUndefinedDependencyError.Consider adding them to the module if they are not already included.
40-41: Confirm Constructor Injection OrderWith the addition of
OutgoingWebhookQueueServiceandOutgoingWebhooksServicein the constructor, ensure that the parameter order aligns with the expected injection order, especially if any parameters are optional or if the project relies on parameter indices for injection.Double-check the dependency injection to prevent any mismatches.
205-215: Review Usage of Type Assertion withas constThe
webhookArgsobject is asserted withas const. This makes all properties readonly and can prevent modifications later in the code.Ensure that making
webhookArgsimmutable is intentional and doesn't affect subsequent operations that may require modifying this object.
| protected initializeWorker() { | ||
| this.worker = new Worker( | ||
| this.queueName, | ||
| async (job: Job<T>) => { | ||
| await this.handleJob(job); | ||
| }, | ||
| { connection: this.connectionOptions }, | ||
| ); | ||
|
|
||
| this.worker?.on('completed', (job: Job) => { | ||
| this.logger.log(`Webhook job ${job.id} completed successfully`); | ||
| }); | ||
|
|
||
| this.worker?.on('failed', (job, error, prev) => { | ||
| this.logger.error( | ||
| `Webhook job ${job?.id} failed after in queue: ${this.queue?.name} retries: ${error.message}`, | ||
| ); | ||
| }); | ||
|
|
||
| this.queue?.on('cleaned', (jobs, type) => { | ||
| this.logger.log(`${jobs.length} ${type} jobs have been cleaned from the webhook queue`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add queue system check in initializeWorker and LGTM on event listeners.
The event listeners for job completion, failure, and queue cleaning are well implemented. However, we should add a check to ensure the queue system is enabled before initializing the worker.
Apply this diff to add the check:
protected initializeWorker() {
+ if (!this.queue) {
+ this.logger.warn('Attempted to initialize worker while queue system is disabled');
+ return;
+ }
this.worker = new Worker(
// ... rest of the method
);
// ... event listeners
}This change prevents attempting to initialize a worker when the queue system is disabled.
The event listener setup is well-implemented, providing good logging for job statuses and queue cleaning.
Committable suggestion was skipped due to low confidence.
| async addJob(jobData: T, jobOptions = {}): Promise<void> { | ||
| await this.queue?.add(this.queueName, jobData, jobOptions); | ||
| } |
There was a problem hiding this comment.
Improve error handling in addJob method.
The current implementation silently fails when the queue is undefined. Implement explicit error handling to make the behavior more predictable and easier to debug.
Apply this diff to improve the addJob method:
async addJob(jobData: T, jobOptions = {}): Promise<void> {
- await this.queue?.add(this.queueName, jobData, jobOptions);
+ if (!this.queue) {
+ this.logger.warn('Attempted to add job while queue system is disabled');
+ throw new Error('Queue system is disabled. Cannot add job.');
+ }
+ try {
+ await this.queue.add(this.queueName, jobData, jobOptions);
+ } catch (error) {
+ this.logger.error(`Failed to add job to queue: ${error.message}`);
+ throw error;
+ }
}This change ensures that callers are aware when jobs cannot be added due to a disabled queue system and provides better error logging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async addJob(jobData: T, jobOptions = {}): Promise<void> { | |
| await this.queue?.add(this.queueName, jobData, jobOptions); | |
| } | |
| async addJob(jobData: T, jobOptions = {}): Promise<void> { | |
| if (!this.queue) { | |
| this.logger.warn('Attempted to add job while queue system is disabled'); | |
| throw new Error('Queue system is disabled. Cannot add job.'); | |
| } | |
| try { | |
| await this.queue.add(this.queueName, jobData, jobOptions); | |
| } catch (error) { | |
| this.logger.error(`Failed to add job to queue: ${error.message}`); | |
| throw error; | |
| } | |
| } |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | ||
| this.connectionOptions = { | ||
| ...REDIS_CONFIG, | ||
| }; | ||
|
|
||
| if (!env.QUEUE_SYSTEM_ENABLED) { | ||
| return; | ||
| } | ||
|
|
||
| this.queue = new Queue(queueName, { | ||
| connection: this.connectionOptions, | ||
| defaultJobOptions: { | ||
| ...Object.entries(QUEUES).find(([_, queueOptions]) => queueOptions.name === queueName)?.[1] | ||
| .config, | ||
| }, | ||
| }); | ||
|
|
||
| if (env.IS_WORKER_SERVICE !== true) { | ||
| this.initializeWorker(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve environment variable checks and add error handling.
- Environment variable checks:
- Convert
env.QUEUE_SYSTEM_ENABLEDandenv.IS_WORKER_SERVICEto booleans for comparison.
- Convert
- Add error handling for queue initialization.
Apply this diff to improve the constructor:
protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) {
this.connectionOptions = {
...REDIS_CONFIG,
};
- if (!env.QUEUE_SYSTEM_ENABLED) {
+ if (env.QUEUE_SYSTEM_ENABLED !== 'true') {
return;
}
- this.queue = new Queue(queueName, {
- connection: this.connectionOptions,
- defaultJobOptions: {
- ...Object.entries(QUEUES).find(([_, queueOptions]) => queueOptions.name === queueName)?.[1]
- .config,
- },
- });
+ try {
+ this.queue = new Queue(queueName, {
+ connection: this.connectionOptions,
+ defaultJobOptions: {
+ ...Object.entries(QUEUES).find(([_, queueOptions]) => queueOptions.name === queueName)?.[1]
+ ?.config,
+ },
+ });
+ } catch (error) {
+ this.logger.error(`Failed to initialize queue: ${error.message}`);
+ return;
+ }
- if (env.IS_WORKER_SERVICE !== true) {
+ if (env.IS_WORKER_SERVICE === 'true') {
this.initializeWorker();
}
}These changes ensure proper boolean comparisons for environment variables and add error handling for queue initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (!env.QUEUE_SYSTEM_ENABLED) { | |
| return; | |
| } | |
| this.queue = new Queue(queueName, { | |
| connection: this.connectionOptions, | |
| defaultJobOptions: { | |
| ...Object.entries(QUEUES).find(([_, queueOptions]) => queueOptions.name === queueName)?.[1] | |
| .config, | |
| }, | |
| }); | |
| if (env.IS_WORKER_SERVICE !== true) { | |
| this.initializeWorker(); | |
| } | |
| } | |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (env.QUEUE_SYSTEM_ENABLED !== 'true') { | |
| return; | |
| } | |
| try { | |
| this.queue = new Queue(queueName, { | |
| connection: this.connectionOptions, | |
| defaultJobOptions: { | |
| ...Object.entries(QUEUES).find(([_, queueOptions]) => queueOptions.name === queueName)?.[1] | |
| ?.config, | |
| }, | |
| }); | |
| } catch (error) { | |
| this.logger.error(`Failed to initialize queue: ${error.message}`); | |
| return; | |
| } | |
| if (env.IS_WORKER_SERVICE === 'true') { | |
| this.initializeWorker(); | |
| } | |
| } |
| if (env.QUEUE_SYSTEM_ENABLED) { | ||
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | ||
| } |
There was a problem hiding this comment.
Improve the evaluation of 'QUEUE_SYSTEM_ENABLED' environment variable.
The current check if (env.QUEUE_SYSTEM_ENABLED) may not behave as expected. Environment variables are strings, so even if env.QUEUE_SYSTEM_ENABLED is set to 'false', it will still be truthy.
To ensure correct behavior, explicitly compare the value:
- if (env.QUEUE_SYSTEM_ENABLED) {
+ if (env.QUEUE_SYSTEM_ENABLED === 'true') {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (env.QUEUE_SYSTEM_ENABLED) { | |
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | |
| } | |
| if (env.QUEUE_SYSTEM_ENABLED === 'true') { | |
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | |
| } |
| const payload = { | ||
| id, | ||
| eventName: 'workflow.context.document.changed', | ||
| apiVersion, | ||
| timestamp: new Date().toISOString(), | ||
| assignee: data.assignee | ||
| ? { | ||
| id: data.assignee.id, | ||
| firstName: data.assignee.firstName, | ||
| lastName: data.assignee.lastName, | ||
| email: data.assignee.email, | ||
| } | ||
| : null, | ||
| assignedAt: data.assignedAt, | ||
| workflowCreatedAt: data.updatedRuntimeData.createdAt, | ||
| workflowResolvedAt: data.updatedRuntimeData.resolvedAt, | ||
| workflowDefinitionId: data.updatedRuntimeData.workflowDefinitionId, | ||
| workflowRuntimeId: data.updatedRuntimeData.id, | ||
| ballerineEntityId: data.entityId, | ||
| correlationId: data.correlationId, | ||
| environment, | ||
| data: data.updatedRuntimeData.context, | ||
| }; |
There was a problem hiding this comment.
Avoid Exposing Sensitive Data in Webhook Payload
The payload includes detailed assignee information and the entire data.updatedRuntimeData.context. There is a risk of exposing sensitive or personally identifiable information (PII).
Review the payload to ensure that only non-sensitive, necessary data is sent. Consider redacting or excluding sensitive fields.
| assignee: data.assignee | ||
| ? { | ||
| id: data.assignee.id, | ||
| firstName: data.assignee.firstName, | ||
| lastName: data.assignee.lastName, | ||
| email: data.assignee.email, | ||
| } | ||
| : null, | ||
| assignedAt: data.assignedAt, |
There was a problem hiding this comment.
Handle Potential Undefined Assignee Properties
When constructing the assignee object, the code assumes that properties like id, firstName, lastName, and email are available. If any of these are undefined, it could lead to unexpected issues.
Add checks or default values to handle cases where these properties might be missing.
| if (env.QUEUE_SYSTEM_ENABLED) { | ||
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | ||
| } | ||
|
|
There was a problem hiding this comment.
Explicit Comparison for Environment Variable
The condition if (env.QUEUE_SYSTEM_ENABLED) relies on the truthiness of env.QUEUE_SYSTEM_ENABLED. If this variable is a string (e.g., 'false'), this condition might not behave as intended.
Use an explicit comparison to ensure correct evaluation:
-if (env.QUEUE_SYSTEM_ENABLED) {
+if (env.QUEUE_SYSTEM_ENABLED === 'true') {This change ensures that the queue system is enabled only when the variable is set to 'true'.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (env.QUEUE_SYSTEM_ENABLED) { | |
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | |
| } | |
| if (env.QUEUE_SYSTEM_ENABLED === 'true') { | |
| return await this.outgoingWebhookQueueService.addJob(webhookArgs); | |
| } | |
| id: data.assignee.id, | ||
| firstName: data.assignee.firstName, | ||
| lastName: data.assignee.lastName, | ||
| email: data.assignee.email, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Assignee Object with Destructuring
You can streamline the assignee object creation using object destructuring if all properties are guaranteed to exist.
-assignee: data.assignee
- ? {
- id: data.assignee.id,
- firstName: data.assignee.firstName,
- lastName: data.assignee.lastName,
- email: data.assignee.email,
- }
- : null,
+assignee: data.assignee
+ ? (({ id, firstName, lastName, email }) => ({ id, firstName, lastName, email }))(data.assignee)
+ : null,Ensure that no additional unintended properties are included.
Committable suggestion was skipped due to low confidence.
| @@ -5,13 +5,15 @@ | |||
| "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.
I think this is better
"setup": "cp .env.example .env && 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",
# Conflicts: # services/workflows-service/package.json # services/workflows-service/prisma/data-migrations
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/redis/const/redis-config.ts (1)
1-9: Update .env.example files with Redis configurationWhile the Redis configuration in this file looks good, it's important to ensure that all
.env.examplefiles in the project are updated to include the necessary Redis environment variables. This helps developers set up the project correctly.Please add the following lines to each
.env.examplefile in the project:REDIS_HOST=localhost REDIS_PORT=6379 REDIS_PASSWORD= REDIS_DB=This will ensure that all required Redis configuration options are documented and available for setup.
services/workflows-service/src/business-report/business-report.module.ts (1)
Line range hint
1-35: Consider refactoring to reduce circular dependencies.While the current implementation works, there are several
forwardRefuses andeslint-disablecomments forimport/no-cycle, indicating potential circular dependencies. This could lead to maintenance challenges in the future.Consider the following suggestions:
- Evaluate if all these circular dependencies are necessary. Can some be resolved by restructuring the code?
- If circular dependencies are unavoidable, consider creating a separate file for shared interfaces or types to break the cycles.
- Document the reasons for circular dependencies, if they must remain, to aid future maintenance.
To help identify the extent of circular dependencies, you can run the following command:
#!/bin/bash # Description: Identify potential circular dependencies # Test: Search for forwardRef and import/no-cycle in the project rg -e "forwardRef" -e "import/no-cycle" services/workflows-service/srcThis will help in assessing the overall architecture and identifying areas for potential refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- services/workflows-service/package.json (4 hunks)
- services/workflows-service/src/business-report/business-report.module.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
- services/workflows-service/src/redis/const/redis-config.ts (1 hunks)
- services/workflows-service/src/workflow/workflow.module.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/workflows-service/package.json
- services/workflows-service/src/env.ts
- services/workflows-service/src/workflow/workflow.module.ts
🧰 Additional context used
🔇 Additional comments (3)
services/workflows-service/src/redis/const/redis-config.ts (1)
1-2: LGTM: Appropriate imports for Redis configurationThe imports are well-chosen for the file's purpose. Using environment variables (
env) for configuration is a good practice, and importingRedisOptionsensures type safety for the Redis configuration.services/workflows-service/src/business-report/business-report.module.ts (2)
Line range hint
1-35: Overall, the changes look good and align with the PR objectives.The addition of the AlertModule to the BusinessReportModule is straightforward and consistent with the existing code structure. It enhances the module's capabilities without introducing any apparent issues.
While there are some architectural considerations regarding circular dependencies, these are pre-existing and not introduced by this change. The current modifications serve the PR's purpose of improving Redis integration and enhancing the service's functionality.
12-12: LGTM: AlertModule integration looks good.The addition of the AlertModule import and its inclusion in the module's imports array is appropriate. This change allows the BusinessReportModule to utilize functionalities provided by the AlertModule, which aligns with the PR's objective of enhancing the service's capabilities.
A few observations:
- The import is correctly placed with other module imports.
- The
forwardRefis used, which is good practice to avoid circular dependencies.- The eslint-disable comment is consistent with other similar imports in this file.
To ensure this change doesn't introduce any unintended side effects, let's verify the AlertModule's usage:
✅ Verification successful
Verified: AlertModule is correctly integrated without introducing circular dependencies. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any direct usage of AlertModule in BusinessReportModule # Test: Search for AlertModule usage in business-report files rg "AlertModule" services/workflows-service/src/business-reportLength of output: 296
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/workflow/workflow.service.unit.test.ts (2)
185-186: Consider using more specific types for new parametersThe addition of two new parameters of type
anyto theDocumentChangedWebhookCallerconstructor might reduce type safety. Consider the following suggestions:
- If possible, use more specific types instead of
anyto improve type checking and code clarity.- If the exact types are not known in the test environment, consider using
unknowninstead ofanyas a safer alternative.- Add comments explaining the purpose of these new parameters to improve code readability.
Example:
new DocumentChangedWebhookCaller( // ... other parameters ... {} as unknown, // TODO: Replace with actual type and explain its purpose {} as unknown, // TODO: Replace with actual type and explain its purpose );
Enhance Type Safety in WorkflowService Unit Tests
The
WorkflowServiceconstructor is being invoked with multiple parameters cast asany, which reduces type safety and may lead to potential runtime errors.Suggestions for improvement:
Use Specific Types: Replace
anywith the actual types or appropriate mock implementations for each dependency.service = new WorkflowService( workflowDefinitionRepo as WorkflowDefinitionRepository, workflowRuntimeDataRepo as WorkflowRuntimeDataRepository, endUserRepo as EndUserRepository, businessReportService as BusinessReportService, {} as EndUserService, // Replace with a proper mock or specific type businessRepo as BusinessRepository, businessService as BusinessService, entityRepo as EntityRepository, customerService as CustomerService, {} as EventEmitter, // Replace with a proper mock or specific type eventEmitter as EventEmitter, testingModule.get(AppLoggerService), projectScopeService as ProjectScopeService, userService as UserService, salesforceService as SalesforceService, workflowTokenService as WorkflowTokenService, uiDefinitionService as UIDefinitionService, {} as RiskRuleService, // Replace with a proper mock or specific type riskRuleService as RiskRuleService, ruleEngineService as RuleEngineService, {} as AdditionalService, // Replace with a proper mock or specific type {} as AnotherService, // Replace with a proper mock or specific type {} as ExtraService, // Replace with a proper mock or specific type );Add Descriptive Comments: Include comments explaining the purpose of each mocked dependency to improve code readability and maintainability.
Group Related Dependencies: Consider grouping related parameters into configuration objects to reduce constructor complexity.
Implementing these improvements will enhance the reliability of your tests and ensure that they accurately reflect the application's behavior.
🔗 Analysis chain
Line range hint
189-214: Improve type safety and clarity in WorkflowService constructorThe addition of multiple parameters of type
anyto theWorkflowServiceconstructor raises some concerns:
- Using
anytype reduces type safety and can lead to potential runtime errors.- The purpose of these new parameters is not clear from the context.
Suggestions for improvement:
- Replace
anywith more specific types orunknownif the exact types are not known in the test environment.- Add comments explaining the purpose of each new parameter.
- Consider grouping related parameters into objects to reduce the number of constructor arguments.
Example:
service = new WorkflowService( workflowDefinitionRepo as WorkflowDefinitionRepo, // Use actual type instead of any // ... other existing parameters ... {} as unknown, // TODO: Replace with actual type and explain its purpose {} as unknown, // TODO: Replace with actual type and explain its purpose {} as unknown, // TODO: Replace with actual type and explain its purpose );Additionally, could you provide more context on the purpose of these new parameters? This information would help in understanding the changes and ensuring they align with the PR objectives.
To verify the impact of these changes, let's check for any related modifications in the WorkflowService implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for WorkflowService constructor in the implementation file rg -A 10 "class WorkflowService" services/workflows-service/src/workflow/workflow.service.tsLength of output: 172
Script:
#!/bin/bash # Search for the WorkflowService constructor in the implementation file rg 'constructor\(' services/workflows-service/src/workflow/workflow.service.tsLength of output: 95
Script:
#!/bin/bash # Retrieve the constructor signature with surrounding lines for context rg -C 5 'constructor\(' services/workflows-service/src/workflow/workflow.service.tsLength of output: 550
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/workflow/workflow.service.unit.test.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
services/workflows-service/src/workflow/workflow.service.unit.test.ts (1)
Line range hint
1-238: Ensure test coverage remains comprehensive with constructor changesWhile the changes to the
DocumentChangedWebhookCallerandWorkflowServiceconstructors don't directly modify the test cases, it's important to consider their potential impact on the overall test coverage:
- Verify that the existing tests still cover all critical paths and edge cases with these new parameters.
- Consider adding new test cases that specifically exercise the functionality related to these new parameters, if applicable.
- Ensure that the mock objects and setup in the
beforeEachblock accurately represent the new dependencies and their behavior.To help ensure comprehensive test coverage, let's check for any new methods or properties added to the WorkflowService that might need testing:
#!/bin/bash # Search for new public methods or properties in WorkflowService rg "public \w+\(|public \w+:" services/workflows-service/src/workflow/workflow.service.tsThis will help identify any new public interfaces that might require additional test cases.
* feat: added dlq logic for each queue * updated queue worker listener logic * feat: added pausing and resuming the workers on destroy * feat: set queue worker logic functionaloity * feat: finalized functionality of base queue and optional dlq * feat: added webhook handling logic
Blokh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
services/workflows-service/src/bull-mq/consts.ts (2)
1-1: Consider using a more general import path.The current import is very specific:
import { BaseJobOptions } from 'bullmq/dist/esm/interfaces';Consider using a more general import path to make the code more resilient to potential changes in the library's structure:
import { BaseJobOptions } from 'bullmq';This change might make the code more maintainable in the long run.
3-14: LGTM! Consider using constants for magic numbers.The structure and configuration of the DEFAULT queue look good. The use of a dead letter queue and an exponential backoff strategy are excellent practices for robust job processing.
To improve code readability and maintainability, consider extracting magic numbers into named constants. For example:
const DEFAULT_ATTEMPTS = 15; const DEFAULT_BACKOFF_DELAY = 1000; export const QUEUES = { DEFAULT: { // ... config: { attempts: DEFAULT_ATTEMPTS, backoff: { type: 'exponential', delay: DEFAULT_BACKOFF_DELAY, }, }, // ... }, };This makes it easier to understand the significance of these values and adjust them if needed.
services/workflows-service/src/bull-mq/bull-mq.module.ts (2)
1-40: LGTM! Consider using type aliases for better readability.The imports and utility functions are well-structured and handle both regular queues and dead-letter queues (DLQs) effectively. The code is type-safe and follows good practices.
For improved readability, consider creating a type alias for
(typeof QUEUES)[keyof typeof QUEUES]. For example:type QueueConfig = (typeof QUEUES)[keyof typeof QUEUES]; function composeQueueAndDlqBoard(queue: QueueConfig) { // ... } const composeInitiateQueueWithDlq = (queue: QueueConfig) => { // ... };This would make the function signatures more concise and easier to understand.
45-58: LGTM! Consider adding error handling for Redis connection.The BullModule configuration is well-structured, using asynchronous setup and centralized Redis configuration. The registration of multiple queues, including DLQs, is handled efficiently.
Consider adding error handling for the Redis connection. For example:
BullModule.forRootAsync({ useFactory: async () => { try { // Test the Redis connection here return { connection: { ...REDIS_CONFIG, }, }; } catch (error) { console.error('Failed to connect to Redis:', error); throw error; // or handle it as appropriate for your application } }, }),This would provide more robust error handling and potentially easier debugging of Redis connection issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- apps/backoffice-v2/src/lib/blocks/components/EditableDetails/EditableDetails.tsx (1 hunks)
- apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx (1 hunks)
- services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/bull-mq.module.ts (1 hunks)
- services/workflows-service/src/bull-mq/consts.ts (1 hunks)
- services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts (1 hunks)
- services/workflows-service/src/bull-mq/types.ts (1 hunks)
- services/workflows-service/src/env.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/env.ts
🧰 Additional context used
🪛 Biome
apps/backoffice-v2/src/lib/blocks/hooks/useDocumentBlocks/useDocumentBlocks.tsx
[error] 412-412: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts
[error] 27-37: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (19)
services/workflows-service/src/bull-mq/types.ts (1)
1-4: LGTM! Consider the optionality of fields.The
TJobPayloadMetadatatype is well-defined and follows TypeScript best practices. It provides a clear structure for job payload metadata, which will enhance type safety and clarity in job processing.Consider if either
projectIdorcustomerNameshould be required fields. If so, you might want to remove the optional modifier (?) from that field. To help verify the usage of this type across the codebase, you can run the following script:This will help you determine if these fields are consistently used and if making either of them required would be appropriate.
✅ Verification successful
Verified: Optional fields are appropriately used.
The
TJobPayloadMetadatatype correctly uses optional fields, allowing flexibility where metadata may not be necessary. No changes are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of TJobPayloadMetadata type across the codebase # Search for usage of TJobPayloadMetadata echo "Searching for usage of TJobPayloadMetadata:" rg "TJobPayloadMetadata" --type ts # Check if projectId or customerName are always provided when using this type echo "Checking if projectId and customerName are always provided:" rg "TJobPayloadMetadata.*=.*\{" -A 3 --type tsLength of output: 2858
services/workflows-service/src/bull-mq/consts.ts (3)
15-34: Please clarify the configuration differences between queues.The INCOMING_WEBHOOKS_QUEUE and OUTGOING_WEBHOOKS_QUEUE configurations are similar to the DEFAULT queue, which is good for consistency. However, there are a few differences that might benefit from explanation:
These queues don't have a
dlq(dead letter queue) property. Is this intentional? If so, how will failed jobs be handled for these queues?The
attemptsvalue is set to 10 for these queues, while it's 15 for the DEFAULT queue. Can you explain the reasoning behind this difference?Providing comments in the code to explain these design decisions would improve maintainability and help future developers understand the rationale behind these configurations.
35-35: LGTM! Good use of type validation.The use of the
satisfiesclause to validate theQUEUESobject against aRecordtype is an excellent practice. It ensures type safety while still allowing for type inference. This approach will catch any future modifications that don't adhere to the expected structure, enhancing code reliability and maintainability.
1-35: Overall, well-structured and type-safe queue configurations.This new file introduces well-organized and type-safe configurations for job queues, which is crucial for robust job processing. The use of TypeScript features, particularly the
satisfiesclause for type validation, is commendable.Key points from the review:
- Consider using a more general import path for
BaseJobOptions.- Extract magic numbers into named constants for better readability.
- Clarify the reasoning behind configuration differences between queues (e.g., absence of
dlqand differentattemptsvalues).These minor improvements and clarifications will further enhance the maintainability and understandability of the code. Great job on implementing these queue configurations!
services/workflows-service/src/bull-mq/incoming-webhook/incoming-webhook-queue.service.ts (3)
1-7: Imports look good and well-structured.The imports are correctly specified and seem to cover all the necessary dependencies for the
IncomingWebhookQueueServiceclass. The use of absolute imports (starting with '@/') indicates a well-organized project structure.
9-9: Well-defined custom type for incoming webhook jobs.The
TJobsWebhookIncomingtype effectively combines the job data (IncomingWebhookData) with metadata (TJobPayloadMetadata). This structure provides a clear and type-safe way to handle incoming webhook jobs.
11-15: Well-structured class definition and constructor.The
IncomingWebhookQueueServiceclass is correctly decorated with@Injectable()for dependency injection. It extendsBaseQueueWorkerServicewith the appropriate generic type, promoting code reuse. The constructor properly initializes the base class with the queue name and logger, following good practices for dependency injection and inheritance.services/workflows-service/src/bull-mq/bull-mq.module.ts (3)
41-44: LGTM! Module structure and exports are well-defined.The module structure is well-organized, with appropriate imports, providers, and exports. Exporting both BullModule and OutgoingWebhookQueueService allows for flexible usage of the queue functionality throughout the application.
Also applies to: 66-70
1-70: Overall, excellent implementation with a few points to address.The BullMqModule is well-structured and implements job queue management effectively using BullMQ. The code is clean, type-safe, and follows good practices for NestJS module development.
Key points to address:
- Security: Implement authentication for the '/queues' route to prevent unauthorized access to the Bull Board interface.
- Error Handling: Consider adding error handling for the Redis connection in the BullModule configuration.
- Type Definitions: For improved readability, consider creating a type alias for the queue configuration type.
These improvements will enhance the robustness and maintainability of the module. Great work overall!
59-65:⚠️ Potential issueSecure the '/queues' route to prevent unauthorized access.
The BullBoardModule configuration is correct, but the '/queues' route is currently exposed without authentication or authorization measures. This could potentially allow unauthorized access to sensitive queue information.
To address this security concern:
Implement authentication middleware for the '/queues' route. You can use NestJS guards or middleware for this purpose.
Consider moving the Bull Board setup to a separate admin module that's only loaded in certain environments or behind additional security layers.
Example of adding a guard:
import { UseGuards } from '@nestjs/common'; import { AuthGuard } from '@nestjs/passport'; @Module({ imports: [ // ... other imports BullBoardModule.forRoot({ route: '/queues', adapter: ExpressAdapter, }), ], }) export class BullMqModule {} // In your app.module.ts or main.ts app.use('/queues', AuthGuard('your-auth-strategy'));This will ensure that only authenticated users can access the queue monitoring interface.
services/workflows-service/src/bull-mq/outgoing-webhook/outgoing-webhook-queue.service.ts (3)
1-21: LGTM: Imports and class declaration are well-structured.The imports are appropriate for the functionality, and the class is properly decorated with
@Injectable(). The constructor correctly initializes the logger and webhookService.
71-79: LGTM:retryJobmethod is well-implemented.The
retryJobmethod correctly handles the retry logic by:
- Calculating the next attempt number.
- Logging the retry information.
- Updating the job progress.
- Moving the job to a delayed state for the next attempt.
This implementation ensures proper tracking and scheduling of retried jobs.
1-80:⚠️ Potential issueAddress potential SSRF vulnerability in webhook processing.
While the overall structure of the
OutgoingWebhookQueueServiceis well-implemented, there's a potential security concern:The service doesn't appear to validate the webhook URLs or methods before making requests. This could lead to Server-Side Request Forgery (SSRF) vulnerabilities if untrusted input is provided in the job data.
To address this, consider implementing URL and method validation in the
handleJobmethod or in theOutgoingWebhooksService.invokeWebhookmethod. Here's a script to verify if such validation exists:If no validation is found, consider adding it to ensure that only allowed URLs and HTTP methods are processed. For example:
private validateWebhookData(data: WebhookJobData) { const allowedDomains = ['example.com', 'api.example.com']; // Configure as needed const allowedMethods = ['GET', 'POST', 'PUT', 'DELETE']; // Configure as needed const url = new URL(data.url); if (!allowedDomains.includes(url.hostname)) { throw new Error('Invalid webhook URL'); } if (!allowedMethods.includes(data.method.toUpperCase())) { throw new Error('Invalid HTTP method'); } } async handleJob(job: Job<TJobArgs>) { this.validateWebhookData(job.data.jobData); // ... rest of the method }This validation helps prevent potential SSRF attacks by ensuring that only trusted domains and methods are used in webhook requests.
🧰 Tools
🪛 Biome
[error] 27-37: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
services/workflows-service/src/bull-mq/base/base-queue-worker.service.ts (5)
1-11: LGTM: Imports and class declaration are well-structured.The imports cover all necessary dependencies, and the class declaration follows NestJS best practices. The use of generics for job data types provides good flexibility.
50-50: LGTM: Abstract method is well-defined.The
handleJobabstract method is correctly defined with proper use of generics and inclusion of metadata in the job type.
69-142: LGTM: Listener methods are well-implemented.The worker and queue listener methods are comprehensive and handle various scenarios effectively. The use of a dead letter queue for permanently failed jobs is a good practice. The error handling and logging in these methods provide valuable insights into job processing.
144-165: LGTM: Lifecycle methods are well-implemented.The
onModuleDestroyandonModuleInitmethods handle resource management appropriately. The pause, close, and resume operations on the queue are well-handled, with good error logging if the queue fails to resume. This ensures proper cleanup and initialization of resources during the module lifecycle.
1-166: Overall, the BaseQueueWorkerService is well-implemented with a few suggestions for improvement.The class provides a robust foundation for queue worker services, handling various aspects of queue management, job processing, error handling, and lifecycle management. The use of generics, abstract methods, and comprehensive listener implementations are commendable.
Key points:
- Consider improving the environment variable check in the constructor.
- Add error handling for queue initialization.
- Enhance the
addJobmethod with explicit error handling.- Add a queue system check in the
initializeWorkermethod.These minor improvements will further enhance the reliability and robustness of the service.
apps/backoffice-v2/src/lib/blocks/components/EditableDetails/EditableDetails.tsx (1)
76-76: Enhancement: Format values consistently usinggetValueThe introduction of
const value = getValue(children);ensures that theDetailcomponent formats the display values appropriately based on their type. This change enhances data consistency and improves the user interface by handling different value types such as dates, date-times, and booleans effectively.
| async handleJob(job: Job<TJobsWebhookIncoming>) { | ||
| this.logger.log(`Processing webhook job ${job.id}`); | ||
|
|
||
| const { service: workingService, payload } = job.data.jobData; | ||
| // TODO - handle the invoking webhook job internally | ||
| } |
There was a problem hiding this comment.
Implement the TODO and add error handling.
The handleJob method has a good start with logging and data extraction. However, there are a few points to address:
- The TODO comment indicates that the core functionality is not yet implemented. This needs to be completed to handle the incoming webhook job.
- There's no error handling. Consider wrapping the job processing in a try-catch block to handle potential errors.
- The method doesn't indicate job completion or failure. Use
job.updateProgress(),job.moveToCompleted(), orjob.moveToFailed()as appropriate.
Here's a suggested structure for improvement:
async handleJob(job: Job<TJobsWebhookIncoming>) {
this.logger.log(`Processing webhook job ${job.id}`);
try {
const { service: workingService, payload } = job.data.jobData;
// TODO: Implement the core logic to handle the incoming webhook job
// ...
// Update progress as needed
// await job.updateProgress(progress);
// Mark job as completed
await job.moveToCompleted('Job processed successfully');
} catch (error) {
this.logger.error(`Error processing job ${job.id}: ${error.message}`);
await job.moveToFailed({ message: 'Job processing failed' });
}
}Would you like me to provide a more detailed implementation for handling the incoming webhook job?
| async handleJob(job: Job<TJobArgs>) { | ||
| this.logger.log(`Processing webhook job ${job.id}`); | ||
|
|
||
| throw new Error('Method not implemented.'); | ||
| const response = await this.webhookService.invokeWebhook({ | ||
| ...job.data.jobData, | ||
| }); | ||
|
|
||
| this.logger.log(`Webhook job ${job.id} completed with status: ${response.status}`); | ||
|
|
||
| if (response.status >= 200 && response.status < 300) { | ||
| return; | ||
| } | ||
|
|
||
| await this.handleRetryStrategy(response.status, job); | ||
| } |
There was a problem hiding this comment.
Remove the throw statement to make the implementation reachable.
The handleJob method contains a throw statement that makes the rest of the method unreachable. This is likely unintended as the actual implementation seems to be present after the throw statement.
Remove the throw statement to make the implementation reachable:
async handleJob(job: Job<TJobArgs>) {
this.logger.log(`Processing webhook job ${job.id}`);
- throw new Error('Method not implemented.');
const response = await this.webhookService.invokeWebhook({
...job.data.jobData,
});
this.logger.log(`Webhook job ${job.id} completed with status: ${response.status}`);
if (response.status >= 200 && response.status < 300) {
return;
}
await this.handleRetryStrategy(response.status, job);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async handleJob(job: Job<TJobArgs>) { | |
| this.logger.log(`Processing webhook job ${job.id}`); | |
| throw new Error('Method not implemented.'); | |
| const response = await this.webhookService.invokeWebhook({ | |
| ...job.data.jobData, | |
| }); | |
| this.logger.log(`Webhook job ${job.id} completed with status: ${response.status}`); | |
| if (response.status >= 200 && response.status < 300) { | |
| return; | |
| } | |
| await this.handleRetryStrategy(response.status, job); | |
| } | |
| async handleJob(job: Job<TJobArgs>) { | |
| this.logger.log(`Processing webhook job ${job.id}`); | |
| const response = await this.webhookService.invokeWebhook({ | |
| ...job.data.jobData, | |
| }); | |
| this.logger.log(`Webhook job ${job.id} completed with status: ${response.status}`); | |
| if (response.status >= 200 && response.status < 300) { | |
| return; | |
| } | |
| await this.handleRetryStrategy(response.status, job); | |
| } |
🧰 Tools
🪛 Biome
[error] 27-37: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
| private async handleRetryStrategy(status: number, job: Job<TJobArgs>) { | ||
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | ||
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | ||
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | ||
| } | ||
|
|
||
| let delayMs: number; | ||
|
|
||
| switch (status) { | ||
| case HttpStatusCode.TooManyRequests: | ||
| case HttpStatusCode.InternalServerError: | ||
| case HttpStatusCode.BadGateway: | ||
| case HttpStatusCode.ServiceUnavailable: | ||
| case HttpStatusCode.GatewayTimeout: | ||
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | ||
| break; | ||
|
|
||
| case HttpStatusCode.RequestTimeout: | ||
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | ||
| break; | ||
|
|
||
| case HttpStatusCode.BadRequest: | ||
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | ||
|
|
||
| default: | ||
| throw new Error(`Webhook job failed with status ${status}: Unexpected Error`); | ||
| } | ||
|
|
||
| await this.retryJob(job, delayMs); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of HTTP status codes in handleRetryStrategy.
The current implementation of handleRetryStrategy has a few areas for improvement:
- Successful status codes (200-299) are not explicitly handled.
- The default case throws an error, which might not be the best approach for unexpected status codes.
Consider updating the method as follows:
private async handleRetryStrategy(status: number, job: Job<TJobArgs>) {
if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) {
this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`);
throw new Error(`Job ${job.id} failed after reaching max attempts`);
}
let delayMs: number;
switch (status) {
+ case HttpStatusCode.Ok:
+ case HttpStatusCode.Created:
+ case HttpStatusCode.Accepted:
+ // Success statuses, no action needed
+ return;
case HttpStatusCode.TooManyRequests:
case HttpStatusCode.InternalServerError:
case HttpStatusCode.BadGateway:
case HttpStatusCode.ServiceUnavailable:
case HttpStatusCode.GatewayTimeout:
delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff
break;
case HttpStatusCode.RequestTimeout:
delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes
break;
case HttpStatusCode.BadRequest:
throw new Error(`Webhook job failed with status ${status}: Bad Request`);
default:
- throw new Error(`Webhook job failed with status ${status}: Unexpected Error`);
+ this.logger.warn(`Unexpected status code ${status} for job ${job.id}. Applying exponential backoff.`);
+ delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff for unexpected errors
+ break;
}
await this.retryJob(job, delayMs);
}This update ensures that successful responses are handled correctly and unexpected status codes are retried with exponential backoff instead of throwing an error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async handleRetryStrategy(status: number, job: Job<TJobArgs>) { | |
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | |
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | |
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | |
| } | |
| let delayMs: number; | |
| switch (status) { | |
| case HttpStatusCode.TooManyRequests: | |
| case HttpStatusCode.InternalServerError: | |
| case HttpStatusCode.BadGateway: | |
| case HttpStatusCode.ServiceUnavailable: | |
| case HttpStatusCode.GatewayTimeout: | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | |
| break; | |
| case HttpStatusCode.RequestTimeout: | |
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | |
| break; | |
| case HttpStatusCode.BadRequest: | |
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | |
| default: | |
| throw new Error(`Webhook job failed with status ${status}: Unexpected Error`); | |
| } | |
| await this.retryJob(job, delayMs); | |
| } | |
| private async handleRetryStrategy(status: number, job: Job<TJobArgs>) { | |
| if (job.opts.attempts && job.attemptsMade >= job.opts.attempts) { | |
| this.logger.warn(`Job ${job.id} reached the maximum retry attempts (${job.opts.attempts})`); | |
| throw new Error(`Job ${job.id} failed after reaching max attempts`); | |
| } | |
| let delayMs: number; | |
| switch (status) { | |
| case HttpStatusCode.Ok: | |
| case HttpStatusCode.Created: | |
| case HttpStatusCode.Accepted: | |
| // Success statuses, no action needed | |
| return; | |
| case HttpStatusCode.TooManyRequests: | |
| case HttpStatusCode.InternalServerError: | |
| case HttpStatusCode.BadGateway: | |
| case HttpStatusCode.ServiceUnavailable: | |
| case HttpStatusCode.GatewayTimeout: | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff | |
| break; | |
| case HttpStatusCode.RequestTimeout: | |
| delayMs = 1000 * 60 * (job.attemptsMade + 1); // Linear backoff in minutes | |
| break; | |
| case HttpStatusCode.BadRequest: | |
| throw new Error(`Webhook job failed with status ${status}: Bad Request`); | |
| default: | |
| this.logger.warn(`Unexpected status code ${status} for job ${job.id}. Applying exponential backoff.`); | |
| delayMs = Math.pow(2, job.attemptsMade + 1) * 1000; // Exponential backoff for unexpected errors | |
| break; | |
| } | |
| await this.retryJob(job, delayMs); | |
| } |
| protected initializeWorker() { | ||
| this.worker = new Worker( | ||
| this.queueName, | ||
| async (job: Job<{ jobData: T; metadata: TJobPayloadMetadata }>) => { | ||
| await this.handleJob(job); | ||
| }, | ||
| { connection: this.connectionOptions }, | ||
| ); | ||
|
|
||
| this.addWorkerListeners(); | ||
| this.addQueueListeners(); | ||
| } |
There was a problem hiding this comment.
Add queue system check in initializeWorker.
Consider adding a check to ensure the queue system is enabled before initializing the worker:
protected initializeWorker() {
+ if (!this.queue) {
+ this.logger.warn('Attempted to initialize worker while queue system is disabled');
+ return;
+ }
this.worker = new Worker(
// ... rest of the method
);
this.addWorkerListeners();
this.addQueueListeners();
}This change prevents attempting to initialize a worker when the queue system is disabled.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected initializeWorker() { | |
| this.worker = new Worker( | |
| this.queueName, | |
| async (job: Job<{ jobData: T; metadata: TJobPayloadMetadata }>) => { | |
| await this.handleJob(job); | |
| }, | |
| { connection: this.connectionOptions }, | |
| ); | |
| this.addWorkerListeners(); | |
| this.addQueueListeners(); | |
| } | |
| protected initializeWorker() { | |
| if (!this.queue) { | |
| this.logger.warn('Attempted to initialize worker while queue system is disabled'); | |
| return; | |
| } | |
| this.worker = new Worker( | |
| this.queueName, | |
| async (job: Job<{ jobData: T; metadata: TJobPayloadMetadata }>) => { | |
| await this.handleJob(job); | |
| }, | |
| { connection: this.connectionOptions }, | |
| ); | |
| this.addWorkerListeners(); | |
| this.addQueueListeners(); | |
| } |
| async addJob(jobData: T, metadata: TJobPayloadMetadata = {}, jobOptions = {}): Promise<void> { | ||
| await this.queue?.add(this.queueName, { metadata, jobData }, jobOptions); | ||
| } |
There was a problem hiding this comment.
Improve error handling in addJob method.
Consider adding explicit error handling when the queue is undefined:
async addJob(jobData: T, metadata: TJobPayloadMetadata = {}, jobOptions = {}): Promise<void> {
- await this.queue?.add(this.queueName, { metadata, jobData }, jobOptions);
+ if (!this.queue) {
+ this.logger.warn('Attempted to add job while queue system is disabled');
+ throw new Error('Queue system is disabled. Cannot add job.');
+ }
+ try {
+ await this.queue.add(this.queueName, { metadata, jobData }, jobOptions);
+ } catch (error) {
+ this.logger.error(`Failed to add job to queue: ${error.message}`);
+ throw error;
+ }
}This change ensures that callers are aware when jobs cannot be added due to a disabled queue system and provides better error logging.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async addJob(jobData: T, metadata: TJobPayloadMetadata = {}, jobOptions = {}): Promise<void> { | |
| await this.queue?.add(this.queueName, { metadata, jobData }, jobOptions); | |
| } | |
| async addJob(jobData: T, metadata: TJobPayloadMetadata = {}, jobOptions = {}): Promise<void> { | |
| if (!this.queue) { | |
| this.logger.warn('Attempted to add job while queue system is disabled'); | |
| throw new Error('Queue system is disabled. Cannot add job.'); | |
| } | |
| try { | |
| await this.queue.add(this.queueName, { metadata, jobData }, jobOptions); | |
| } catch (error) { | |
| this.logger.error(`Failed to add job to queue: ${error.message}`); | |
| throw error; | |
| } | |
| } |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | ||
| this.connectionOptions = { | ||
| ...REDIS_CONFIG, | ||
| }; | ||
|
|
||
| if (!env.QUEUE_SYSTEM_ENABLED) { | ||
| return; | ||
| } | ||
|
|
||
| const currentQueue = Object.entries(QUEUES).find( | ||
| ([_, queueOptions]) => queueOptions.name === queueName, | ||
| ); | ||
|
|
||
| if (!currentQueue) { | ||
| throw new Error(`Queue configuration of ${queueName} not found in QUEUES`); | ||
| } | ||
|
|
||
| const queueConfig = currentQueue[1]; | ||
| this.queue = new Queue(queueName, { | ||
| connection: this.connectionOptions, | ||
| defaultJobOptions: { | ||
| ...queueConfig.config, | ||
| }, | ||
| }); | ||
|
|
||
| this.deadLetterQueue = | ||
| 'dlq' in queueConfig | ||
| ? new Queue(queueConfig.dlq, { connection: this.connectionOptions }) | ||
| : undefined; | ||
|
|
||
| this.initializeWorker(); | ||
| } |
There was a problem hiding this comment.
Improve environment variable check and consider error handling for queue initialization.
- The environment variable check should compare to 'true' as a string:
- if (!env.QUEUE_SYSTEM_ENABLED) {
+ if (env.QUEUE_SYSTEM_ENABLED !== 'true') {- Consider adding error handling for queue initialization:
+ try {
this.queue = new Queue(queueName, {
connection: this.connectionOptions,
defaultJobOptions: {
...queueConfig.config,
},
});
+ } catch (error) {
+ this.logger.error(`Failed to initialize queue: ${error.message}`);
+ return;
+ }These changes will ensure proper boolean comparison for the environment variable and add robustness to queue initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (!env.QUEUE_SYSTEM_ENABLED) { | |
| return; | |
| } | |
| const currentQueue = Object.entries(QUEUES).find( | |
| ([_, queueOptions]) => queueOptions.name === queueName, | |
| ); | |
| if (!currentQueue) { | |
| throw new Error(`Queue configuration of ${queueName} not found in QUEUES`); | |
| } | |
| const queueConfig = currentQueue[1]; | |
| this.queue = new Queue(queueName, { | |
| connection: this.connectionOptions, | |
| defaultJobOptions: { | |
| ...queueConfig.config, | |
| }, | |
| }); | |
| this.deadLetterQueue = | |
| 'dlq' in queueConfig | |
| ? new Queue(queueConfig.dlq, { connection: this.connectionOptions }) | |
| : undefined; | |
| this.initializeWorker(); | |
| } | |
| protected constructor(protected queueName: string, protected readonly logger: AppLoggerService) { | |
| this.connectionOptions = { | |
| ...REDIS_CONFIG, | |
| }; | |
| if (env.QUEUE_SYSTEM_ENABLED !== 'true') { | |
| return; | |
| } | |
| const currentQueue = Object.entries(QUEUES).find( | |
| ([_, queueOptions]) => queueOptions.name === queueName, | |
| ); | |
| if (!currentQueue) { | |
| throw new Error(`Queue configuration of ${queueName} not found in QUEUES`); | |
| } | |
| const queueConfig = currentQueue[1]; | |
| try { | |
| this.queue = new Queue(queueName, { | |
| connection: this.connectionOptions, | |
| defaultJobOptions: { | |
| ...queueConfig.config, | |
| }, | |
| }); | |
| } catch (error) { | |
| this.logger.error(`Failed to initialize queue: ${error.message}`); | |
| return; | |
| } | |
| this.deadLetterQueue = | |
| 'dlq' in queueConfig | |
| ? new Queue(queueConfig.dlq, { connection: this.connectionOptions }) | |
| : undefined; | |
| this.initializeWorker(); | |
| } |
| @@ -409,6 +409,7 @@ export const useDocumentBlocks = ({ | |||
| } | |||
|
|
|||
| if (ocrResult?.parsedData?.[title]) { | |||
| debugger; | |||
There was a problem hiding this comment.
Remove the debugger statement to prevent unintended pauses in execution.
The debugger; statement on line 412 can cause the application to halt unexpectedly, which is undesirable in production code.
Apply this diff to remove the debugger statement:
- debugger;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| debugger; |
🧰 Tools
🪛 Biome
[error] 412-412: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const commitMessage = response.choices[0].message.content | ||
| .trim() | ||
| .replace(/`/g, '') | ||
| .replace(/"/g, '\\"') | ||
| .replace(/\$/g, '\\$'); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that all occurrences of backslashes are properly escaped in the commit message. This can be achieved by adding a replacement step for backslashes using a regular expression with the global flag (g). This ensures that all backslashes in the string are escaped, preventing any potential injection vulnerabilities.
| @@ -69,2 +69,3 @@ | ||
| .trim() | ||
| .replace(/\\/g, '\\\\') | ||
| .replace(/`/g, '') |
| const commitMessage = response.choices[0].message.content | ||
| .trim() | ||
| .replace(/`/g, '') | ||
| .replace(/"/g, '\\"') |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that backslashes in the input are properly escaped. This can be done by adding a .replace(/\\/g, '\\\\') to the existing chain of replacements. This will ensure that all backslashes are escaped correctly.
| @@ -69,2 +69,3 @@ | ||
| .trim() | ||
| .replace(/\\/g, '\\\\') | ||
| .replace(/`/g, '') |
| const response = await axios.get(`${env.UNIFIED_API_URL}/merchants/analysis/${id}`, { | ||
| params: { | ||
| customerId, | ||
| }, | ||
| headers: { | ||
| Authorization: `Bearer ${env.UNIFIED_API_TOKEN}`, | ||
| }, | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to ensure that the id parameter is validated and sanitized before being used in constructing the URL for the outgoing HTTP request. One way to achieve this is to use a whitelist of allowed id values or to validate the id against a specific pattern that ensures it cannot be manipulated to perform SSRF attacks.
The best way to fix the problem without changing existing functionality is to introduce a validation step for the id parameter before it is used in the URL. This can be done by defining a validation schema using a library like zod and applying this schema to the id parameter.
| @@ -150,2 +150,8 @@ | ||
| public async findById({ id, customerId }: { id: string; customerId: string }) { | ||
| const idSchema = z.string().regex(/^[a-zA-Z0-9_-]+$/); | ||
| const validationResult = idSchema.safeParse(id); | ||
| if (!validationResult.success) { | ||
| throw new errors.ValidationError('Invalid id format'); | ||
| } | ||
|
|
||
| try { |
feat: added redis defaults
Summary by CodeRabbit
Release Notes
New Features
BullMqModulefor managing job queues.BaseQueueWorkerServicefor structured job processing.Bug Fixes
Documentation
Chores