feat: improve logger injection in constructor #380
Conversation
WalkthroughThe pull request introduces a consistent approach to logger initialization across multiple services and classes in the API. The changes modify the constructor signatures of various services and classes to provide a default logger using the class name. This standardization simplifies logger instantiation by removing the requirement to explicitly pass a logger instance, while ensuring each class has a contextually named logger for improved traceability and debugging. Changes
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
apps/api/src/modules/notifications/notifications.controller.ts (1)
Line range hint
31-32: Review logging of sensitive notification data.The debug log contains raw notification data which might include PII. Consider implementing data masking.
- this.logger.debug(`Notification Request Data: ${JSON.stringify(notificationData)}`); + this.logger.debug('Notification Request Data:', { + ...notificationData, + message: notificationData.message ? '[MASKED]' : undefined, + to: notificationData.to ? '[MASKED]' : undefined + });apps/api/src/common/guards/api-key/api-key.guard.ts (1)
Line range hint
29-63: Consider refactoring to reduce code duplication.The validation logic for HTTP and GraphQL requests contains duplicate code.
Consider extracting the common validation logic:
+ private async validateApiKeyAndProviderId( + headers: Record<string, string>, + providerId: number + ): Promise<boolean> { + const serverApiKeyHeader = headers['x-api-key']; + return await this.validateApiKeyHeader(serverApiKeyHeader, providerId); + } + async validateRequest(execContext: ExecutionContext): Promise<boolean> { const request = execContext.switchToHttp().getRequest(); this.logger.debug( `Request validation started for request body: ${JSON.stringify(request.body)}`, ); // Get api key header incase of http request if (request && request.headers) { - this.logger.debug(`Fetching request header and provider ID: ${request.body.providerId}`); - const serverApiKeyHeader = request.headers['x-api-key']; - this.logger.debug('Fetching provider Id'); const requestProviderId = request.body.providerId; - const validationResult = await this.validateApiKeyHeader( - serverApiKeyHeader, - requestProviderId, - ); + const validationResult = await this.validateApiKeyAndProviderId( + request.headers, + requestProviderId + ); if (validationResult) { return true; } } // Get api key header incase of graphql request const ctx = GqlExecutionContext.create(execContext); const req = ctx.getContext().req; - this.logger.debug( - `Fetching request header and provider ID for GraphQL: ${req.body.providerId}`, - ); - const serverApiKeyHeader = req.headers['x-api-key']; const requestProviderId = request.body.providerId; - const validationResult = await this.validateApiKeyHeader(serverApiKeyHeader, requestProviderId); + const validationResult = await this.validateApiKeyAndProviderId( + req.headers, + requestProviderId + ); if (validationResult) { return true; }
🧹 Nitpick comments (8)
apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (1)
Line range hint
21-33: Excellent error handling with appropriate logging!The try-catch blocks with error logging provide good traceability for debugging. Consider adding error codes or identifiers for better error tracking.
} catch (error) { - this.logger.error('Error assigning SNS configuration', error); + this.logger.error('Error assigning SNS configuration', { + errorCode: 'SNS_CONFIG_ERROR', + error: error.message + }); throw error;Also applies to: 44-49
apps/api/src/modules/providers/wa360dialog/wa360dialog.service.ts (1)
54-54: LGTM! Consider enhancing error logging structure.The logger initialization is consistent with other services. However, the error logging in the sendMessage method could be improved.
Consider structuring the error logging to include more context:
-this.logger.error(`Error sent from provider: ${providerId}`, providerResponseError); +this.logger.error('Provider request failed', { + providerId, + error: providerResponseError, + endpoint: this.apiUrl +});apps/api/src/modules/providers/wa-twilio-business/wa-twilio-business.service.ts (2)
41-41: LGTM! Consider standardizing error handling.The logger initialization is consistent with other services. However, the error handling could be standardized across all Twilio services.
Consider adopting a consistent error handling pattern across all Twilio services:
-throw new Error(`Failed to fetch delivery status: ${error.message}`); +this.logger.error('Failed to fetch delivery status', { + providerId, + sid, + error: error.message +}); +throw error;
Line range hint
37-54: Consider creating a base provider service.Given the similar patterns across all provider services (logger initialization, error handling, transport assignment), consider creating an abstract base provider service to standardize these common behaviors.
This would help:
- Standardize error handling across all providers
- Provide consistent logging patterns
- Reduce code duplication
- Simplify testing
Example structure:
abstract class BaseProviderService { protected constructor( protected readonly providersService: ProvidersService, protected readonly logger: Logger ) { this.logger = new Logger(this.constructor.name); } protected handleError(error: any, context: object) { this.logger.error('Provider operation failed', { ...context, error }); throw error; } }apps/api/src/modules/providers/sms-plivo/sms-plivo.service.ts (1)
Line range hint
84-92: Consider standardizing error handling structure.The error handling differentiates between provider API errors and other errors, but could be more structured.
Consider this pattern:
} catch (error) { - if (error.apiID) { - // Log relevant parts of the error response - this.logger.error(`Error sent from provider: ${providerId}`, error.message); - throw error; - } else { - // Handle cases where there is no response (network issues, etc.) - throw new Error(`Failed to send message: ${error.message}`); - } + if (!error.apiID) { + throw new Error(`Network or configuration error: ${error.message}`); + } + this.logger.error({ + message: `Provider error for ID: ${providerId}`, + error: { + apiId: error.apiID, + message: error.message + } + }); + throw error; }apps/api/src/modules/providers/mailgun/mailgun.service.ts (1)
Line range hint
82-89: Consider enhancing attachment error handling.The attachment processing could benefit from more detailed error handling and validation.
Consider adding validation and more specific error types:
if (attachment.path) { try { const filepath = path.resolve(attachment.path); + // Validate file exists before reading + await fs.access(filepath); data = await fs.readFile(filepath); } catch (error) { + if (error.code === 'ENOENT') { + throw new BadRequestException(`File not found at path: ${attachment.path}`); + } throw new BadRequestException( `Failed to read file at path: ${attachment.path}: ${error.message}`, ); } }apps/api/src/modules/providers/vc-twilio/vc-twilio.service.ts (1)
90-90: LGTM! Consider enhancing error logging.The logger initialization change looks good and aligns with the PR objectives. However, consider enhancing error logging in the catch blocks to include the error stack trace for better debugging.
} catch (error) { - throw new Error(`Failed to send voice call: ${error.message}`); + this.logger.error('Failed to send voice call', { error }); + throw new Error(`Failed to send voice call: ${error.message}`); }apps/api/src/common/decorators/is-data-valid.decorator.ts (1)
33-33: LGTM! Consider enhancing validation error logging.The logger initialization change is consistent with the PR objectives. However, the validation error handling could benefit from improved logging.
if (errors.length > 0) { const validationErrors = errors.map((obj) => Object.values(obj.constraints)); + this.logger.debug('Validation failed', { errors: validationErrors }); throw new BadRequestException(...validationErrors); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/api/src/common/decorators/is-data-valid.decorator.ts(1 hunks)apps/api/src/common/guards/api-key/api-key.guard.ts(1 hunks)apps/api/src/modules/notifications/notifications.controller.ts(1 hunks)apps/api/src/modules/providers/aws-ses/aws-ses.service.ts(1 hunks)apps/api/src/modules/providers/mailgun/mailgun.service.ts(1 hunks)apps/api/src/modules/providers/push-sns/push-sns.service.ts(1 hunks)apps/api/src/modules/providers/sms-kapsystem/sms-kapsystem.service.ts(1 hunks)apps/api/src/modules/providers/sms-plivo/sms-plivo.service.ts(1 hunks)apps/api/src/modules/providers/sms-sns/sms-sns.service.ts(1 hunks)apps/api/src/modules/providers/sms-twilio/sms-twilio.service.ts(1 hunks)apps/api/src/modules/providers/smtp/smtp.service.ts(1 hunks)apps/api/src/modules/providers/vc-twilio/vc-twilio.service.ts(1 hunks)apps/api/src/modules/providers/wa-twilio-business/wa-twilio-business.service.ts(1 hunks)apps/api/src/modules/providers/wa-twilio/wa-twilio.service.ts(1 hunks)apps/api/src/modules/providers/wa360dialog/wa360dialog.service.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/api/src/modules/notifications/notifications.controller.ts (1)
Learnt from: LakshayaT
PR: OsmosysSoftware/osmo-x#293
File: apps/api/src/modules/notifications/notifications.controller.ts:15-18
Timestamp: 2024-11-12T07:51:34.599Z
Learning: The `addNotificationsToQueue` method in the `notificationService` includes comprehensive error handling, logging errors, and managing the state of the queue processing.
🔇 Additional comments (14)
apps/api/src/modules/providers/smtp/smtp.service.ts (2)
11-11: LGTM! Logger initialization follows NestJS best practices.The default logger initialization with class name improves log filtering and debugging capabilities.
Line range hint
14-27: Consider masking sensitive information in logs.The SMTP configuration contains sensitive credentials. While the current implementation doesn't explicitly log these values, it's worth adding a precautionary note.
Run this script to check for potential credential logging:
✅ Verification successful
Current logging implementation is secure
The code only logs operational status messages at debug level and does not expose any sensitive SMTP configuration data. The existing implementation already follows security best practices for logging.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential credential logging rg -i "password|credentials|secret" -C 3 "logger\.(debug|log|info|warn|error)"Length of output: 156
Script:
#!/bin/bash # Search for potential credential logging with fixed patterns echo "=== Searching for general credential logging ===" rg -i "(password|credential|secret|auth)" --type ts -C 3 "\.(?:debug|log|info|warn|error)\(" echo -e "\n=== Searching for SMTP-specific logging ===" rg -i "smtp" --type ts -C 3 "\.(?:debug|log|info|warn|error)\(" echo -e "\n=== Searching for config/transport logging ===" rg -i "(config|transport)" --type ts -C 3 "\.(?:debug|log|info|warn|error)\("Length of output: 779
Script:
#!/bin/bash # Search in the SMTP service directory echo "=== Searching for logger usage in SMTP service ===" cd apps/api/src/modules/providers/smtp rg "logger" -A 3 -B 3 --type ts echo -e "\n=== Searching for config logging ===" rg "smtpConfig" -A 3 -B 3 --type ts echo -e "\n=== Checking all TypeScript files in SMTP directory ===" find . -type f -name "*.ts" -exec cat {} \;Length of output: 4106
apps/api/src/modules/providers/push-sns/push-sns.service.ts (2)
16-16: LGTM! Consistent with the standardized logger initialization pattern.The implementation aligns with the project-wide logger injection improvements.
Line range hint
20-29: Review AWS credential handling in logs.Similar to SMTP service, ensure AWS credentials are never logged, even in debug mode.
Run this script to verify credential handling:
✅ Verification successful
AWS credentials are properly handled in logs
The code follows secure practices with no credential logging. Debug logs only contain operational status messages without exposing sensitive information:
- "Started assigning SNS push notification client"
- "Sending SNS push notification"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for AWS credential logging rg -i "aws.*key|credentials|secret" -C 3 "logger\.(debug|log|info|warn|error)"Length of output: 156
Script:
#!/bin/bash # Search for potential AWS credential logging patterns rg -t ts -t js "logger\." -C 3 | rg -i "aws.*key|credentials|secret" # Additional specific search for AWS credential related logging rg -t ts -t js -C 3 'this\.logger\.(debug|log|info|warn|error).*(\baws\b|\bcredentials\b|\bsecret\b|\baccessKey\b)'Length of output: 188
Script:
#!/bin/bash # Find and show the push-sns service file content fd -t f "push-sns.service.ts" -x cat {} # Search for all logger usage in TypeScript files containing 'sns' rg -t ts "this\.logger\." -C 3 $(fd -t f "sns.*\.ts$")Length of output: 5426
apps/api/src/modules/providers/sms-sns/sms-sns.service.ts (1)
16-16: LGTM! Logger initialization is consistent with other services.The implementation maintains consistency across the codebase.
apps/api/src/modules/notifications/notifications.controller.ts (2)
12-12: LGTM! Logger initialization follows the standardized pattern.The implementation aligns with the project-wide logger improvements.
Line range hint
38-43: Good error handling with detailed logging!The error handling follows best practices by:
- Preserving HttpException
- Logging detailed error information
- Including stack trace for debugging
apps/api/src/modules/providers/sms-kapsystem/sms-kapsystem.service.ts (1)
37-37: LGTM! Consider documenting testing implications.The logger initialization with class name context improves log filtering. However, this change might affect unit tests that need to verify logging behavior.
For testing scenarios where log verification is needed, you can still override the logger:
const mockLogger = new Logger(); const service = new SmsKapsystemService(httpService, providersService, mockLogger);apps/api/src/modules/providers/sms-twilio/sms-twilio.service.ts (1)
39-39: LGTM! Consistent with logger injection pattern.The implementation follows the same pattern as other services, maintaining consistency across the codebase.
apps/api/src/modules/providers/wa-twilio/wa-twilio.service.ts (1)
39-39: LGTM! Consider updating tests if needed.The default logger initialization improves code consistency. Ensure any existing tests that explicitly pass a logger instance are updated accordingly.
Run this script to check for affected tests:
✅ Verification successful
No test impact found, but consider adding tests
The logger initialization change is safe as there are no existing tests that could be affected. However, since no tests were found for the WaTwilioService, consider adding unit tests to improve coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that might need updates rg -l "new WaTwilioService.*Logger" --type tsLength of output: 47
Script:
#!/bin/bash # Find test files related to WaTwilioService echo "=== Searching for WaTwilioService test files ===" rg -l "WaTwilioService" --type ts | grep -i "test\|spec" echo -e "\n=== Checking service instantiation in tests ===" rg "new WaTwilioService" -A 5 -B 5 --type ts echo -e "\n=== Checking for any mock/stub usage ===" rg "WaTwilioService" -A 3 -B 3 --type ts | grep -i "mock\|stub\|test"Length of output: 488
Script:
#!/bin/bash # Search for any Twilio-related test files echo "=== Searching for Twilio-related test files ===" fd -t f "test|spec" | rg -l "twilio" -i echo -e "\n=== Checking test directory structure ===" fd -t d "test|spec|__tests__"Length of output: 298
apps/api/src/modules/providers/sms-plivo/sms-plivo.service.ts (1)
55-55: LGTM! Logger initialization is consistent with other services.The default logger initialization using the class name improves code consistency.
apps/api/src/modules/providers/mailgun/mailgun.service.ts (1)
24-24: LGTM! Logger initialization is consistent.The default logger initialization using the class name improves code consistency.
apps/api/src/common/guards/api-key/api-key.guard.ts (1)
21-21: LGTM! Logger initialization is consistent.The default logger initialization using the class name improves code consistency.
apps/api/src/modules/providers/aws-ses/aws-ses.service.ts (1)
29-29: LGTM! Well-structured logging implementation.The logger initialization change is consistent with the PR objectives. The class demonstrates good logging practices with debug logs for tracking execution flow and error logs for exception cases.
API PR Checklist
Pre-requisites
.env.examplefile with the required values as applicable.PR Details
PR details have been updated as per the given format (see below)
feat: add admin login endpoint)Additional Information
ready for reviewshould be added if the PR is ready to be reviewed)Note: Reviewer should ensure that the checklist and description have been populated and followed correctly, and the PR should be merged only after resolving all conversations and verifying that CI checks pass.
Description:
Related changes:
N/A
Screenshots:
N/A
Query request and response:
N/A
Documentation changes:
N/A
Test suite/unit testing output:
N/A
Pending actions:
N/A
Additional notes:
N/A
Summary by CodeRabbit
Refactor
Chores